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

fill_init_params makes assumptions about model parameter names - is that safe? #107

Open
akabla opened this issue Apr 10, 2021 · 9 comments
Labels
invalid This doesn't seem right seeking opinions

Comments

@akabla
Copy link
Member

akabla commented Apr 10, 2021

Looking at fill_init_params in processing.jl, I realise that we are assuming specific symbols for the exponents of the multiple springpot models. I think this is problematic as one would assume these symbols are arbitrary. The logic used to set these parameters is unclear too.

It may be more appropriate to require that initial values are provided by the user, or included in the model definition itself (but fundamentally the values of anything that has a physical unit depend on the data, not the model - only fract exponents may benefit from default values). In processing, maybe we should only verify that the parameters satisfy the constraints as otherwise we would have to make some assumptions in code that I believe should be generic.

@akabla akabla added invalid This doesn't seem right seeking opinions labels Apr 10, 2021
@moustachio-belvedere
Copy link
Member

moustachio-belvedere commented Apr 10, 2021

If I remember correctly, that function is not very important. I wrote it as a helper function to simply fill out other initial conditions if a user doesn't want to provide them all. I don't think it's a major issue, but I understand your concern.

The worst case I can envisage is that there is some future model that decides to name two springpots using the opposite naming convention, so that the auto-filled initial conditions are actually the wrong way round. I don't even know if this always a problem from memory, some of those two springpot models end up being symmetric with respect to their parameters anyway. It could be more pressing if we have a much smarter constraints system during optimization such as that I mentioned in #86

In other scenarios I can imagine, a more harmless name clash is possible, but if the user has not specified initial guesses for those parameters then I can't imagine they mind if it is auto-set to 0.25, 0.5, 0.75 or anything else?

Edit: noticed it raises a warning aswell, in case a user does care what defaults are but forgot to specify

If, on further investigation it seems more trouble than it's worth to keep that functionality, we should just cut it out and raise an error if not all parameters' initial guesses are provided by the user. Otherwise, if you can come up with a more generic way to do it then go for it, but currently the only way I can see we keep track of how many springpots are in a model is by their naming convention.

@moustachio-belvedere
Copy link
Member

I like your idea of including suggested initial parameters within each model, one risk there though. If the model has a condition a>b (for example) and user sets a=0.01, and the model's default for b is 0.25 then it doesn't meet constraints.

@moustachio-belvedere
Copy link
Member

Feel free to delete if you are concerned, I don't feel strongly on it 👍

@moustachio-belvedere
Copy link
Member

And yes, the parameters depend on the data fundamentally. In practice, we often don't have a good sense for what sensible parameters are on first fitting iteration though so there is some benefit.

@akabla
Copy link
Member Author

akabla commented Apr 10, 2021

Good point re: default values in the model and the user setting one of them such that it clashes with the default value of the other. Best to make it a requirement then to specify initial values, and then check constraints are met and through an error when needed.

@moustachio-belvedere
Copy link
Member

moustachio-belvedere commented Apr 10, 2021

If empty, it was setting everything to 0.5 before this change it seems, with an index based fix for springpot parameters, probably why I tried to make something more robust:

https://github.com/JuliaRheology/RHEOS.jl/blame/41cfc646d34685379e38ff193d9d1ea5cd624f65/src/processing.jl#L207

@moustachio-belvedere
Copy link
Member

moustachio-belvedere commented Apr 10, 2021

On second look. it seems I just was probably just trying to extend a similar approach but for partially provided init conditions, hence the additional complexity.

Feel free to force user-provided params, equally valid to me 👍

@moustachio-belvedere
Copy link
Member

On this though, I disagree:

the values of anything that has a physical unit depend on the data, not the model - only fract exponents may benefit from default values

A counter-example: Reynolds number doesn't have physical units but depends on data and describes something inherently physical. The same is true for springpot coefficients.

@akabla
Copy link
Member Author

akabla commented Apr 10, 2021

I'm not saying that dimensionless parameters don't depend on the data - they would too. In the context of our rheological models, the fractional exponents are likely to be bounded by 0 and 1 in most practical cases, and therefore a default value at 0.5 does bother me as much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right seeking opinions
Projects
None yet
Development

No branches or pull requests

2 participants