Conversation
Collaborator
|
Regarding the testing, I tried to make a set of generic class instances for
standardizing the class method tests.
Not sure it worked out so well though.
I'll have a look at it.
…On Wed, 1 Jul 2020 at 20:55, Guy Davies ***@***.***> wrote:
I have finally added the MvN prior that I had been threatening to do.
Turns out it is fast (expected).
I have polished the code a little and the docs should be up to @nielsenmb
<https://github.com/nielsenmb> 's standards (largely in part to ctrl-c
ctrl-v).
We should run some tests in different corners of the numax parameter space
but I am mildly optimistic.
Note - I have not attempted to account for the observational uncertainty
on the prior values (the same is true for kde) as this softens things up a
little.
I cannot persuade the tests to pass. I have a problem that in pbjam_tests
the kde type is hard coded in as a type but with no methods. This causes 2
tests to fail - I'm not really sure how to get round this! What do there
tests actually do if the kde class is just a shell with no methods?
@nielsenmb <https://github.com/nielsenmb> - could you look at the failing
tests and see how they could be fixed?
------------------------------
You can view, comment on, or merge this pull request online at:
#243
Commit Summary
- Added MVN to priors
- Polishing MvN class
File Changes
- *M* Examples/Example-Star-API-Kepler.ipynb
<https://github.com/grd349/PBjam/pull/243/files#diff-50c4dc44118896c6f1a9ba194a636846>
(368)
- *M* Inspector/pbjam_check_list.csv
<https://github.com/grd349/PBjam/pull/243/files#diff-04d1d62f7f6051b2830251ae66b1cf09>
(32918)
- *M* pbjam/asy_peakbag.py
<https://github.com/grd349/PBjam/pull/243/files#diff-1ad6c1c9b68bccbc34404c65f131035a>
(223)
- *M* pbjam/priors.py
<https://github.com/grd349/PBjam/pull/243/files#diff-485921a6c1965db47185290017c23aed>
(259)
- *M* pbjam/star.py
<https://github.com/grd349/PBjam/pull/243/files#diff-88067ec6395a4e5a28e9924a66ade166>
(91)
- *M* pbjam/tests/pbjam_tests.py
<https://github.com/grd349/PBjam/pull/243/files#diff-7e7650e2b11fdfeaa98bd9dcb0089e94>
(138)
- *M* pbjam/tests/test_priors.py
<https://github.com/grd349/PBjam/pull/243/files#diff-1f3feb63c8f5e12a6dcb9e5b854bf52a>
(82)
Patch Links:
- https://github.com/grd349/PBjam/pull/243.patch
- https://github.com/grd349/PBjam/pull/243.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#243>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEJWO35BM3QOVRJYLRUCE6TRZOIETANCNFSM4ON52HGQ>
.
|
Collaborator
Author
|
Super - Thanks @nielsenmb |
Collaborator
Author
|
Any progress on this? @nielsenmb |
Collaborator
|
Can you copy in the error message you get from pytest?
…On Mon, 20 Jul 2020 at 10:37, Guy Davies ***@***.***> wrote:
Any progress on this? @nielsenmb <https://github.com/nielsenmb>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#243 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEJWO32WEE2NVOQGCBOMJ3DR4QF6PANCNFSM4ON52HGQ>
.
|
Collaborator
|
It looks like this branch doesn't have the Travis update. The tests are
still run by Pytest.
I'd suggest merging the latest PR #243 into master and then pulling that
into the MvN PR.
We should maybe talk about this tomorrow if there's time after the cico.
On Mon, 20 Jul 2020 at 10:55, Martin Nielsen <
martinbonielsen.astro@gmail.com> wrote:
… Can you copy in the error message you get from pytest?
On Mon, 20 Jul 2020 at 10:37, Guy Davies ***@***.***> wrote:
> Any progress on this? @nielsenmb <https://github.com/nielsenmb>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#243 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEJWO32WEE2NVOQGCBOMJ3DR4QF6PANCNFSM4ON52HGQ>
> .
>
|
Collaborator
|
Managed to get a look at the branch, and the pytest failure doesn't look
directly related to the MvN addition.
All tests are running fine on master, so if you pull that into the MvN PR
then it might fix the issue.
On Mon, 20 Jul 2020 at 11:09, Martin Nielsen <
martinbonielsen.astro@gmail.com> wrote:
… It looks like this branch doesn't have the Travis update. The tests are
still run by Pytest.
I'd suggest merging the latest PR #243 into master and then pulling that
into the MvN PR.
We should maybe talk about this tomorrow if there's time after the cico.
On Mon, 20 Jul 2020 at 10:55, Martin Nielsen <
***@***.***> wrote:
> Can you copy in the error message you get from pytest?
>
> On Mon, 20 Jul 2020 at 10:37, Guy Davies ***@***.***>
> wrote:
>
>> Any progress on this? @nielsenmb <https://github.com/nielsenmb>
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#243 (comment)>, or
>> unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AEJWO32WEE2NVOQGCBOMJ3DR4QF6PANCNFSM4ON52HGQ>
>> .
>>
>
|
Collaborator
Author
|
Thanks - I'll follow you advice when I have a sec ... :) What is the time line for merging this into master - after publication? |
Collaborator
|
Yeah, that's probably best, although technically what the referee should be
evaluating (if they evaluate the code at all) is what's on Pypi.
…On Mon, 20 Jul 2020 at 11:45, Guy Davies ***@***.***> wrote:
Thanks - I'll follow you advice when I have a sec ... :)
What is the time line for merging this into master - after publication?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#243 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEJWO3YTC4JJZV5EC6D5OTDR4QN57ANCNFSM4ON52HGQ>
.
|
Collaborator
|
Don't think this is relevant anymore, can it be closed? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I have finally added the MvN prior that I had been threatening to do. Turns out it is fast (expected).
I have polished the code a little and the docs should be up to @nielsenmb 's standards (largely in part to ctrl-c ctrl-v).
We should run some tests in different corners of the numax parameter space but I am mildly optimistic.
Note - I have not attempted to account for the observational uncertainty on the prior values (the same is true for kde) as this softens things up a little.
I cannot persuade the tests to pass. I have a problem that in pbjam_tests the kde type is hard coded in as a type but with no methods. This causes 2 tests to fail - I'm not really sure how to get round this! What do there tests actually do if the kde class is just a shell with no methods?
@nielsenmb - could you look at the failing tests and see how they could be fixed?