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

Fixes/methods on AdlerData. #80

Merged
merged 7 commits into from
Mar 19, 2024
Merged

Fixes/methods on AdlerData. #80

merged 7 commits into from
Mar 19, 2024

Conversation

astronomerritt
Copy link
Contributor

@astronomerritt astronomerritt commented Mar 12, 2024

Fixes #70.

  • AdlerData.print_data() will clearly print out the data stored in AdlerData.
  • Wrote a unit test for this method.

Fixes #74.

  • AdlerData.get_phase_parameters_in_filter() returns a dictionary of the phase parameters in a specified filter and model. Chose a dictionary for clarity here.
  • Wrote a unit test for this method.

Fixes #72.

  • The unpopulated class attributes on AdlerData now fill with nans on initialisation, to make it clear there is no real data in them yet. The exception is nobs_adler, which is an array of ints and therefore initialises with zeros.

Fixes #73.

  • The user can now use AdlerData.populate_phase_parameters() to update only select parameters. If not updating the full set, simply specify the arguments and (if necessary) the model in the function call.
  • For example: test_object.populate_phase_parameters("u", "model_1", 11.0, 12.0, 13, 14.0, 15.0, 16.0, [17.0], [18.0]) will update/populate everything, as before, but test_object.populate_phase_parameters("u", model_name="model_2", H=25.0, H_err=26.0, parameters=[27.0, 27.5], parameters_err=[28.0, 28.5]) will add a new model, model_2, and its parameters, without changing any of the others. Existing models can also be updated.
  • Updated the unit tests.

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 requested a review from jrob93 March 12, 2024 13:32
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.40%. Comparing base (b080dda) to head (a675c40).

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

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #80       +/-   ##
===========================================
+ Coverage   69.84%   86.40%   +16.56%     
===========================================
  Files           3        3               
  Lines          63      103       +40     
===========================================
+ Hits           44       89       +45     
+ Misses         19       14        -5     

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

@astronomerritt astronomerritt marked this pull request as draft March 12, 2024 15:42
@astronomerritt astronomerritt marked this pull request as ready for review March 12, 2024 16:13
@jrob93
Copy link
Collaborator

jrob93 commented Mar 13, 2024

I noticed get_phase_parameters_in_filter returns "phase_parameters" and "phase_parameters_err" whereas populate_phase_parameters accepts "parameters" and "parameters_err". I've added a very minor commit to update everything to be like "phase_parameter", this way the output of get can be used directly in populate e.g:

dat_dict = test.AdlerData.get_phase_parameters_in_filter("y")
test.AdlerData.populate_phase_parameters("y",**dat_dict)

@jrob93
Copy link
Collaborator

jrob93 commented Mar 13, 2024

Just some minor suggestions before merging. In AdlerData default None values lead to very small/large numbers being stored when variable is not set, print_data therefore displays output even when a filter has not been set.

@jrob93
Copy link
Collaborator

jrob93 commented Mar 13, 2024

Also get_phase_parameters_in_filter breaks if there is no model and so you cannot retrieve observation metadata:

test_ad = AdlerData(["y"])
dat_dict = {"phaseAngle_min":2,
           "phaseAngle_range":10,
           "nobs":100,
           "arc":30}
test_ad.populate_phase_parameters("y",**dat_dict)
test_ad.get_phase_parameters_in_filter("y")

E.g. you might start populating AdlerData with just metadata before fitting a model. Expected behaviour would be to return the metadata and empty values for model variables? Just adding a model_name makes it work, but maybe we want to force people to do this

test_ad = AdlerData(["y"])
dat_dict = {"phaseAngle_min":2,
           "phaseAngle_range":10,
           "nobs":100,
           "arc":30}
test_ad.populate_phase_parameters("y",**dat_dict)
test_ad.populate_phase_parameters("y",model_name = "HG")
test_ad.get_phase_parameters_in_filter("y")

@astronomerritt
Copy link
Contributor Author

astronomerritt commented Mar 14, 2024

I've changed AdlerData.get_phase_parameters_in_filter() to instead force the user to specify a model if they want model-dependent parameters. If no model is supplied, only the model-independent metadata is returned (and a message is printed to the terminal informing them of this).

I think this is much clearer/better behaviour. Good catch!

I have also changed how the arrays are initialised. Instead of np.empty I'm using np.fill and filling them with nans. The exception to this is nobs_adler, which is an array of ints. Can't cast np.nan as int.

The alternative is to use dtype=object (and filling arrays with None). However, I'd then probably have to write type-checking, as there would be nothing to stop someone setting a value to "cheese" or a dictionary or an entire function or something equally silly.

Copy link
Collaborator

@jrob93 jrob93 left a comment

Choose a reason for hiding this comment

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

Hey just tested the changes and they work great thanks. Defaulting to zeros for number of observations and the get_phase_parameters_in_filter and print_data makes sense to me!

@astronomerritt astronomerritt merged commit f976298 into main Mar 19, 2024
6 checks passed
@astronomerritt astronomerritt deleted the AdlerData_fixes branch March 19, 2024 13:36
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