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

New phase class #695

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

New phase class #695

wants to merge 6 commits into from

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented May 9, 2020

In #685, I mentioned that I had previously attempted to replace PINT's underpowered Phase class with something I had written with Jing for our scintillometry package. It is a Quantity subclass which separately tracks integer and fractional cycles (bit like TimeDelta). It behaves like a Quantity but tries to keep precision, using, e.g., just the fractional part when evaluating np.sin.

I revived my old attempt, rebasing and fixing the few places where things broke. Right now, it is slightly hacked in - I think that if it were to replace phase everywhere, things would likely simplify.

@mhvk mhvk marked this pull request as draft May 9, 2020 00:00
@mhvk mhvk force-pushed the new_phase_class branch from 50b556a to 18e0c49 Compare May 9, 2020 00:26
@mhvk
Copy link
Contributor Author

mhvk commented May 9, 2020

p.s. Not working on python 2.7, obviously...

@mhvk
Copy link
Contributor Author

mhvk commented May 13, 2020

Moving final discussion from #586 here, in particular the answer to #685 (comment):

So, at the telecon, we decided to merge this PR. But, we are very interested in studying @mhvk's Phase class as well. We are wondering if a version of it that kept many of the nice features (that are missing in our simple Phase class) but didn't use the u.cycle unit might be worthwhile?

I think it would not be particularly difficult for the class itself have the equivalency between cycles and dimensionless built-in. Indeed, the example I pushed in #695 already did that for initialization, and that could be extended to the unit-conversion methods .to and .to_value. I don't think I'm the right person to making a PR that extends its use to other parts of tempo, but I'm definitely willing to help trouble-shoot problems and make adjustments as needed.

I do think that the same phase class (or a lower-precision version) should be used for orbital phase as well.

mhvk added 6 commits May 24, 2020 14:37
The new Phase class has better precision and rounds to even.
The former brings out very small floating point differences.
At floating point precision, sadly, 0.7 - 1 != -0.3.
@mhvk mhvk force-pushed the new_phase_class branch from 18e0c49 to a7cd6fe Compare May 24, 2020 20:23
@mhvk
Copy link
Contributor Author

mhvk commented May 24, 2020

I rebased this now that #701 has been merged. Not surprisingly, the multiplication and division work fine with my phase class, though I had to adjust quite a number of the tests since the current PINT phase class does not round to even, and also loses precision in some cases (which amusingly, for the tests gives more reasonable results, such as 0.7 - 1 == 0.3, while if one works with floats directly:

In [5]: 0.7 - 1 == -0.3      
Out[5]: False

Since my class keeps full float precision, those tests failed... It also means that, e.g., test_associative_scalar_addition had to be rewritten to use numbers for which this is actually true. It is generally not true for floats:

In [6]: (0.4+0.3) - 0.5 == 0.4 + (0.3-0.5)  
Out[6]: False

In [7]: (0.4+0.3) - 0.5 - (0.4 + (0.3-0.5))                   
Out[7]: -5.551115123125783e-17

In [8]: 2**-54
Out[8]: 5.551115123125783e-17

cc @paulray, @champagne-cmd

p.s. Also added the full test suite; if there is serious interest in having this class, I'd probably like to redo the PR to include the git history of its creation. I'm happy to relicense under BSD.

@paulray paulray added the tabled label Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants