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

Initial commit of AdlerData class. #65

Merged
merged 9 commits into from
Mar 8, 2024
Merged

Initial commit of AdlerData class. #65

merged 9 commits into from
Mar 8, 2024

Conversation

astronomerritt
Copy link
Collaborator

@astronomerritt astronomerritt commented Mar 5, 2024

Fixes #53.

  • Created the AdlerData class, which will store all calculated AdlerData values.
  • AdlerPlanetoid now creates an instance of the AdlerData class as a class attribute on initialisation
  • Wrote a method, AdlerData.populate_phase_parameters(), which should allow for easy population.
  • Wrote a unit test.

Fixes #68.

  • Changed instances of sys.exit() to raise Exception() instead.

Fixes #67:

  • num_obs has been added to the Observations object.

Fixes #63.

  • Fixed a typo.

Fixes #64.

  • Removed unnecessary .to_table() call.

Review Checklist for Source Code Changes

  • Does pip install still work?
  • Have you written a unit test for any new functions?
  • Do all the units tests run successfully?
  • Does adler run successfully on a test set of input files/databases?
  • Have you used black on the files you have updated to confirm python programming style guide enforcement?

@astronomerritt astronomerritt marked this pull request as ready for review March 5, 2024 14:49
@astronomerritt astronomerritt requested a review from jrob93 March 5, 2024 14:49
@astronomerritt astronomerritt marked this pull request as draft March 5, 2024 16:12
@astronomerritt astronomerritt requested a review from mschwamb March 7, 2024 14:39
@astronomerritt
Copy link
Collaborator Author

Updated to make recommended changes.

@astronomerritt
Copy link
Collaborator Author

...and forgot to push. Done now.

@astronomerritt astronomerritt marked this pull request as ready for review March 7, 2024 14:57
@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 87.27273% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 72.46%. Comparing base (1b9e784) to head (3d8620f).
Report is 3 commits behind head on main.

Files Patch % Lines
src/adler/dataclasses/AdlerData.py 87.27% 7 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##           main      #65       +/-   ##
=========================================
+ Coverage      0   72.46%   +72.46%     
=========================================
  Files         0        3        +3     
  Lines         0       69       +69     
=========================================
+ Hits          0       50       +50     
- Misses        0       19       +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mschwamb
Copy link
Collaborator

mschwamb commented Mar 7, 2024

instead of phase_parameter1 and phase_parameter2 what about

phase_parameters= [[parameter1,parameter2], [parameter1],] if we happen to be running multiple models.

@astronomerritt
Copy link
Collaborator Author

I was trying to avoid too many nested lists - this would mean that phase_parameter is nested three times, for filter, model and then parameter. I will still do this if you think it's best, but I worry about maintainability when someone who isn't me has to read the code.

@mschwamb
Copy link
Collaborator

mschwamb commented Mar 7, 2024

phase_parameter_err is a variable twice

@mschwamb
Copy link
Collaborator

mschwamb commented Mar 7, 2024

Fair enough,but is nested better than having nulls or empty arrays when you don't have a parameter2 for 1 model?

@astronomerritt
Copy link
Collaborator Author

Mistake in the docstring only, they're separate in the function. Correcting.

@astronomerritt
Copy link
Collaborator Author

Fair enough,but is nested better than having nulls or empty arrays when you don't have a parameter2 for 1 model?

Point taken. I can make this change, one second.

@astronomerritt
Copy link
Collaborator Author

Done. In retrospect, now I've made the change I prefer it...

Copy link
Collaborator

@mschwamb mschwamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@astronomerritt astronomerritt merged commit 610d429 into main Mar 8, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants