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

[WIP] Display/return quantities with units #30

Merged
merged 4 commits into from
Mar 17, 2020
Merged

Conversation

mstimberg
Copy link
Member

@mstimberg mstimberg commented Mar 16, 2020

Internally, all values are unitless, but now values that are returned to the user are displayed with units. This affects:

  • the returned parameters and errors from Fitter.fit and Fitter.refine
  • the values stored in Fitter.best_params and Fitter.best_error
  • the values returned by Fitter.results (except when using dataframe, since it doesn't support units)
  • The parameters/error displayed in the callback functions

This of course might break some existing scripts (or give weird results if they multiply things with units again). You can create your Fitter with use_units=False to switch back to the previous behaviour. I think that given that the package is still "beta" and not widely used (I guess), it is still ok to break things this way. I prefer doing this now and having use_units=True as the default instead of having to tell users to add an option to get the nicer behaviour.

There's one thing that still needs to be fixed: the callback gets the normalized error value, but the callback does not know about this and will display it wrong. I'm not quite sure what's the best solution here. Would it be more natural for the callback to show the error normalized without units (e.g. if the actual error is 2mV and your normalization is 1mV, it would be displayed as 2), or should it revert the normalization, i.e. always show the same error independent of normalization? @romainbrette any thoughts?

Closes #18, closes #19

@romainbrette
Copy link
Member

I would think that normalized error is better. The most important point, in any case, is that it is consistent between the population-based method and the gradient method.

@mstimberg
Copy link
Member Author

This is what it looks like now:

with use_units=True (default)

no normalization (normalization=1):

... (error: 37.62624665 mV^2)

normalization=1*mV:

... (error: 37.62624665)

normalization=1e-3:

(error: 37.62624665 V^2)

 

with use_units=False

no normalization (normalization=1):

... (error: 3.762624665e-05)

normalization=1*mV or normalization=1e-3 (no difference) :

... (error: 37.62624665)

Using a normalization without units and calc_units=True certainly looks odd (3rd example above), but in a way it makes sense? We could also simply not allow a normalization without units if you use units in general. What do you think?

Either way all this is consistent between fit and refine.

@romainbrette
Copy link
Member

I think it's all fine, it's what I would expect.

@romainbrette romainbrette merged commit 8ccb68e into master Mar 17, 2020
@mstimberg mstimberg deleted the return_units branch March 17, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Online display of best parameters is not readable Units of results
2 participants