FIx instrument model

[henrikjacobsenfys]

Fixes #135

I planned to also work on #101 , hence the name of the branch, but I need to think more about it. For now, it's good to fix this bug.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.74%. Comparing base (860c3ca) to head (161e723).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #136      +/-   ##
===========================================
+ Coverage    97.64%   97.74%   +0.10%     
===========================================
  Files           37       37              
  Lines         2460     2486      +26     
  Branches       413      421       +8     
===========================================
+ Hits          2402     2430      +28     
+ Misses          34       32       -2     
  Partials        24       24              

Flag	Coverage Δ
integration	0.00% <0.00%> (ø)
unittests	97.74% <100.00%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[seventil]

As I see from the API of InstrumentModel, ResolutionModel and BackgroundModel, Q can be set to None in each of those. Default value is also None for all of their init parameter Q. When you set Q in the InstrumentModel it propagates to InstrumentModel and ResolutionModel. But if you set Q to None in the InstrumentModel afterwards, the value will not be propagated to ResolutionModel and BackgroundModel, creating a discrepancy in state between the three models.
I understand that your workflows might avoid such situation, but the API itself allows it. Is that an issue? Why even allow to set Q property to None?

[henrikjacobsenfys]

Well, the issue is that the expected workflow is something like this:

# new background model, can be given a Q but we don't have to
background_model=BackgroundModel(components=background_components) 
instrument_model=InstrumentModel(
background_model=background_model,
resolution_model=previous_fit.instrument_model.resolution_model) 
# resolution model has specific components with specific values for each Q 
#determined by a fit. InstrumentModel does not yet know about these Q
# InstrumentModel is (and should be, I think) responsible for making sure 
#resolution_model has the right Q. But right now, instrument_model's Q is None, so that's 
# what it passes on to its resolution_model, resetting it, which I don't want


# new sample model, can also be given a Q, but doesn't need it
sample_model=SampleModel(components=sample_components) 
experiment=Experiment(data=some_data) # contains information about Q

# Analysis passes the Q from experiment to its instrument_model,
# which as mentioned passes it on to its resolution_model and background_model
# If the Q from Analysis matches what is already in resolution_model, then it
# does not overwrite its components.
analysis=Analysis(experiment=experiment,
sample_model=sample_model,
instrument_model=instrument_model) 

I probably need a more sophisticated way of handling the state of these things, but it's kind of difficult. I don't want users to have to think about supplying their InstrumentModel with the correct Q from the Experiment - that's the job of Analysis. So this intermediate step where the InstrumentModel is created and a ResolutionModel is passed to it is kind of tricky.

One potential solution is to get rid of InstrumentModel and just pass a SampleModel, BackgroundModel, ResolutionModel directly to Analysis. This does make Analysis into a quite large class, but it may be the best solution? Thoughts are very welcome.

In this case, the workflow is

background_model=BackgroundModel(components=background_components) 
resolution_model=previous_fit.instrument_model.resolution_model) 
sample_model=SampleModel(components=sample_components) 
experiment=Experiment(data=some_data) # contains information about Q

analysis=Analysis(
experiment=experiment,
background_model=background_model,
sample_model=sample_model,
resolution_model=resolution_model
) 

Hmm, the more I think about it, the more I like it. I had it like this at first, then changed it, and I don't remember why anymore...

[henrikjacobsenfys]

Thinking a bit more..

perhaps the solution is to initialize Q to None. Then if self.Q is not None, the Q setter throws an error unless the new Q is identical to what's already there.

I can add a clear_Q or destroy_Q or something method to allow changing Q, that makes it clear that it breaks what's in the model.

[rozyczko]

minor issues raised

[rozyczko]

should these not be asserts?

[rozyczko]

Should those also have a conditional for if self._Q is not None?

[henrikjacobsenfys]

Not quite done yet (missing unit tests, for one), but updated so that Q cannot be set if it's not None. This avoids many of the potential problems.

[henrikjacobsenfys]

@seventil and @rozyczko Would you take a look at this again? :)

[seventil]

LGTM for me. Text below is something you might want to take into consideration

I am a bit confused by current implementation. I think the best way to undertand the implementation requirements is to derive them from use-case requirements. As far as I understand them there are two workflows:

1. InstrumentModel is instantiated, ResolutionModel holds the previous Q (is there always a previous?)
2. InstrumentModel gets (non-None) Q from the experiment, Qs across InstrumentModel/ResolutionModel/BackgroundModel are synchronised.

Assuming these two, there is no reason for InstrumentModel Q setter to accept None values. Just make the Q value in Q setter non-optional. (as you kinda do with return, but the code doesn't set None to this Q i think anyway, so the whole branch with accepting Q as value but then not changing anything is unnecessary anyway imho)

What is the Q getter from InstrumentModel used for? - you can know whether or not it's a problem if getters output is optional None. If it's not used, why have it as a property even? If it's used - what would InstrumentModel.Q==None mean for the code that's reading it? If it's ok, I'd just self._Q=None in init jus to have _Q attr in the object, and then use the setter-route for any valid (non-None) Q assignment either from init attribute or if updated from Experiment.

[rozyczko]

I'm fine with the code

[henrikjacobsenfys]

LGTM for me. Text below is me yapping to myself to understand what's going on.

I am a bit confused by current implementation. I think the best way to undertand the implementation requirements is to derive them from use-case requirements. As far as I understand them there are two workflows:

1. InstrumentModel is instantiated, ResolutionModel holds the previous Q (is there always a previous?)

There is not always a previous Q, which is part of the difficulty

2. InstrumentModel gets (non-None) Q from the experiment, Qs across InstrumentModel/ResolutionModel/BackgroundModel are synchronised.

Yes, that is the idea. If it matters, it gets it from Analysis.

Assuming these two, there is no reason for InstrumentModel Q setter to accept None values.

I don't know if this is a good reason, but my thinking was this: what happens if the user removes an Experiment from their Analysis? This is allowed, but then Q becomes None, which gets passed to the InstrumentModel. I could of course make a check in Analysis and prevent it, but I deemed it simpler to allow it without errors. Perhaps this falls under the "never fail silently" and should be changed.

Just make the Q value in Q setter non-optional. (as you kinda do with return, but the code doesn't set None to this Q i think anyway, so the whole branch with accepting Q as value but then not changing anything is unnecessary anyway imho)

Again, not saying this is optimal, but I do want to be able to change Experiment, which (I think) should update Q everywhere, except

What is the Q getter from InstrumentModel used for?

I didn't think much about it - I made a setter, so I also made a getter.

• you can know whether or not it's a problem if getters output is optional None. If it's not used, why have it as a property even? If it's used - what would InstrumentModel.Q==None mean for the code that's reading it? If it's ok, I'd just self._Q=None in init jus to have _Q attr in the object, and then use the setter-route for any valid (non-None) Q assignment either from init attribute or if updated from Experiment.

Perhaps we can chat a bit about this tomorrow? It's clearly not a perfect implementation yet, but here's what I'm trying to make happen:

Analysis takes an Experiment, SampleModel and InstrumentModel, which itself contains a BackgroundModel and ResolutionModel. Everything except the Experiment is optional for the following. The Experiment is the source of truth for Q.

All the SomethingModel contain a list of either Parameter or ComponentCollection or both, one for each Q. They should be automatically generated from a template, since I don't want users to copy/paste their model to every Q manually.

However, if the SomethingModels have been fitted or changed by the user, I don't want them to be overwritten by accident. This is why setting to None does nothing.

It should be easy to transfer any and all SomethingModels to a new Analysis and supplement with new ones where Analysis uses the template to generate all the components. It should also be easy to give the Analysis a new Experiment.

Finally, it should be easy to work with all of these things outside Analysis, mostly for testing out ideas for the users. So I want users to be able to set Q of their InstrumentModel without an Experiment.