lorenzo-rovigatti / oxDNA

A new version of the code to simulate the oxDNA/oxRNA models, now equipped with Python bindings
https://dna.physics.ox.ac.uk/
GNU General Public License v3.0
38 stars 26 forks source link

Add 'precision' non-mandatory option for ForceEnergy and PotentialEnergy observables #100

Closed mlsample closed 1 month ago

mlsample commented 2 months ago

I had previously sent in a pull request where I had removed the %s from the split fmt, leading to a bug. All cases have been checked, this time, where each case has given me the intended behavior. Apologies for the confusion.

Motivation: the energy of an external force applied to a particle in an origami size system becomes 0.0 with the currently implemented precision when divided out by the number of particles, especially when you have multiple groups. Having the option to change the precision of Potential Energy obs is nice for when you split the contributions.

lorenzo-rovigatti commented 2 months ago

Thanks for the PR! What do you think about using %g instead of any combination of %X.Ylf? It could be probably used as default since it would work well for most cases. Can you test it and see if it works for you?

mlsample commented 2 months ago

Good idea. One question, my instinct tells me that with %g, which defaults to 6 significant digits, would be sufficient in which case the optional argument of precision may not be needed. Would you recommend removing the optional argument 'precision' to keep the code as is but flexible to small numbers (with %g), or keep the argument in case for whatever reason there would be a need to increase precision in the future?

lorenzo-rovigatti commented 2 months ago

With this change some of tests fail because of the different way energies are printed (see here). I'm a great proponent of backwards compatibility, so I'd rather not go directly this route of changing an output by default. I propose we add bool _general_format = false and int _precision = 6 to BaseObservable, with both being initialised by BaseObservable's get_settings. These new variables would be (optionally) used by classes that inherit from BaseObservable. With this structure the code in PotentialEnergy.cpp' andForceEnergy.cpp` would become something like

if (_general_format) {
  return Utils::sformat("%." + precision + "g", U);
}
else {
  return Utils::sformat("% 10." + _precision + "lf", U);
}

or something similar. What do you think? I can try to add this change myself and make a new PR that you could test. This would make it easier to extend this behaviour to other observables.

mlsample commented 2 months ago

I like it! Backwards compatible while increasing generality, good thinking. I implemented your idea, but if you think its suboptimal, I would be more than willing to make changes, or test a version you write.

mlsample commented 2 months ago

Also this time I ran make test_run and make test_quick and they passed all tests. Will make sure to do so before I push again in the future.

lorenzo-rovigatti commented 2 months ago

I think we are almost there. I think that we can strengthen it a bit if we define _precision as an int (so that no non-sense gets printed if the user uses a non-integer value), and then maybe define another member _number_formatter that is %10.6lf by default and gets overwritten if _general_format and _precision are specified in the input file, and it's then used by observables to print. Something like this (NB: I haven't tested it):

if(_general_precision) {
  _number_formatter = Utils::sformat("%%.%dg", _precision);
}
else {
  _number_formatter = Utils::sformat("%%10.%dlf", _precision);
}

which can be used like

return Utils::sformat(_number_formatter, U);

What do you think?

mlsample commented 2 months ago

I like it. Typing _precision is a great idea to make the code more robust. _number_formatter is a better generalization increasing maintainability. I'll send in a tested pull as soon as I can.

mlsample commented 1 month ago

Sorry for taking so long to push this, only took a few minutes to modify the code which I think verbatim was what you wrote. Either way, I would still be willing to make further improvements if you have ideas.

lorenzo-rovigatti commented 1 month ago

Thanks again for the PR!