Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Groups integrations updates for pandeia/pandexo v1.4 #365

Draft
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

bourque
Copy link
Contributor

@bourque bourque commented May 14, 2020

This PR updates the groups & integrations calculator with various updates needed for pandeia/pandexo version 1.4.

@pep8speaks
Copy link

pep8speaks commented May 14, 2020

Hello @bourque, Thank you for updating !

Line 193:21: W605 invalid escape sequence '('
Line 193:25: W605 invalid escape sequence ')'
Line 193:57: W605 invalid escape sequence '\c'
Line 193:74: W605 invalid escape sequence '\e'
Line 199:46: W605 invalid escape sequence '('
Line 199:48: W605 invalid escape sequence '\l'
Line 199:56: W605 invalid escape sequence '\m'
Line 199:66: W605 invalid escape sequence '\h'
Line 199:79: W605 invalid escape sequence '\m'
Line 199:85: W605 invalid escape sequence ')'
Line 200:46: W605 invalid escape sequence '('
Line 200:48: W605 invalid escape sequence '\l'
Line 200:56: W605 invalid escape sequence '\m'
Line 200:66: W605 invalid escape sequence '\h'
Line 200:79: W605 invalid escape sequence '\m'
Line 200:85: W605 invalid escape sequence ')'
Line 201:46: W605 invalid escape sequence '('
Line 201:48: W605 invalid escape sequence '\l'
Line 201:56: W605 invalid escape sequence '\m'
Line 201:66: W605 invalid escape sequence '\h'
Line 201:79: W605 invalid escape sequence '\m'
Line 201:85: W605 invalid escape sequence ')'
Line 207:58: W605 invalid escape sequence '('
Line 207:62: W605 invalid escape sequence '\m'
Line 207:69: W605 invalid escape sequence '\m'
Line 207:75: W605 invalid escape sequence ')'
Line 517:42: E261 at least two spaces before inline comment
Line 523:1: E302 expected 2 blank lines, found 1
Line 528:52: E231 missing whitespace after ','
Line 533:69: E261 at least two spaces before inline comment
Line 533:69: E262 inline comment should start with '# '
Line 534:5: E265 block comment should start with '# '

Line 49:1: E124 closing bracket does not match visual indentation
Line 58:1: E124 closing bracket does not match visual indentation

Comment last updated at 2020-10-13 19:51:06 UTC

@bourque bourque changed the title [WIP] Groups integrations updates for pandeia/pandexo v1.4 Groups integrations updates for pandeia/pandexo v1.4 May 21, 2020
@bourque bourque requested a review from nespinoza May 28, 2020 14:29
@bourque
Copy link
Contributor Author

bourque commented May 28, 2020

@nespinoza Could you review this? I think this is good to go.

I tested the groups and integrations calculator locally, through the web app, and everything seems to be working fine. I'm not sure if the results that are being returned are reasonable, however. Please let me know if you have any suggestions on how to test that out and make sure that they are reasonable.

This PR does add unit tests that make sure the contents and the structure of the groups_integrations.json file are what is expected. There is also one test in there that ensures that the output of the perform_calculation() function in groups_integrations.py is also what is expected. If you can think of any other tests to add, just let me know!

Lastly, it seems that before, the web app was using the groups_integrations_input_data.json file located within the data/ directory of the package, and not the groups_integrations.json file within the $EXOCTK_DATA package -- not sure why that is, but I switched it so that it now uses a file named groups_integrations.json within the data/ directory.

@nespinoza
Copy link
Collaborator

Hey @bourque --- this is fantastic, thanks for all the work on this! We need to validate this against the JWST ETC I believe before merging for both bright-end targets (e.g., WASP-18) and faint-end targets (e.g., TRAPPIST-1). If you can do this, that would be great, but if you are not familiar with the JWST ETC I can do this tomorrow.

@bourque
Copy link
Contributor Author

bourque commented May 28, 2020

@nespinoza Perhaps we could do a Slack call tomorrow and test it together? That way I could learn how to better navigate the JWST ETC and hopefully be able to do this myself in the future.

@nespinoza
Copy link
Collaborator

That sounds like a plan @bourque! Let's decide for a time internally :-).

@bourque
Copy link
Contributor Author

bourque commented Oct 9, 2020

Moving this Issue for the release 1.2 project, as there are some result descrepencies to figure out first.

@bourque bourque changed the base branch from master to develop November 4, 2020 17:12
@hover2pi
Copy link
Member

hover2pi commented Nov 9, 2020

@nespinoza @bourque Does this need a validation test before merging to make sure the results are what we expect? Is there a way to do it without involving the ETC so we can have CI test it with each build?

@bourque bourque marked this pull request as draft February 16, 2021 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants