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

Fix u.cycle bugs on astropy 4.0 #616

Merged
merged 7 commits into from
Feb 28, 2020
Merged

Conversation

rossjjennings
Copy link
Member

This is the result of following up on the weird behavior @paulray noticed in #615. Certain operations on quantities with the u.cycle unit were causing them to be implicitly made dimensionless, i.e. converted to radians by multiplying by 2π. This turns out not to be an astropy 4.0-specific behavior, but it was causing bugs on astropy 4.0 because the u.cycle unit was being propagated further than it had been. I found that the weird behavior could be traced to this line, which sets the u.dimensionless_angles() equivalency globally.

This PR gets rid of this behavior by deleting the offending line. However, doing this caused quite a few test failures due to the resulting stricter unit checks. The follow-up commits fix these failures, which turned out to have four different causes:

  1. Some of the scripts, including photonphase.py, had relied on np.where() silently stripping the u.cycle unit from its arguments. I fixed these instances by explicitly stripping the units with a line like phases=phases.to(u.cycle).value before the call to np.where().
  2. Some derivative-calculating functions in the binary model classes relied on the u.rad unit being treated as equivalent to dimensionless units. I fixed these instances by wrapping the offending code in a with u.set_enabled_equivalencies(u.dimensionless_angles()) block.
  3. The phase-calculating functions in the IFunc and Wave model components explicitly converted dimensionless quantities (in radians) into the u.cycle unit. I fixed these by going directly to cycles, rather converting from radians.
  4. The delay-calculating function in the SolarWindDispersion model component took the inverse cosine of a dimensionless Quantity, which in astropy 4.0 results in a Quantity with the u.rad unit. Without u.dimensionless_angles(), this lead to unit incompatibilities later. I fixed this by converting the dimensionless Quantity into a regular np.ndarray before taking the inverse cosine.

With these changes, all of the tests now pass on astropy 4.0! So I added a final commit removing astropy <4.0 requirement from requirements.txt, requirements_dev.txt, and setup.cfg.

@rossjjennings rossjjennings changed the title Remove global dimensionless angles equivalency and fix resulting bugs Fix u.cycle bugs on astropy 4.0 Feb 24, 2020
@paulray
Copy link
Member

paulray commented Feb 24, 2020

Nice work!

BTW, I think we should delete htest_optimize.py from PINT. It is a quick hack I made and isn't worth maintaining.

@rossjjennings
Copy link
Member Author

OK. I've been bold and added a commit removing it.

@luojing1211
Copy link
Member

Do we need htest functionality in PINT?

@rossjjennings
Copy link
Member Author

What do we mean by htest functionality?

@luojing1211
Copy link
Member

@paulray mentioned the htest_optimize.py is a quick hack. But should we have a more serious htest code in PINT? or should it be independent?

@rossjjennings
Copy link
Member Author

rossjjennings commented Feb 24, 2020

@luojing1211 I understood that much. I just don't know what "htest code" is supposed to do.

Maybe we should have a new issue for it if it's something we want to add?

@paulray
Copy link
Member

paulray commented Feb 24, 2020

Given that PINT is really useful for processing event mode data from many spacecraft, it is approppriate that we have H-test calculation routines (among others) available in pint.eventstats. However, the htest_optimize.py script was a quick hack that I wrote that wasn't being maintained and so I suggested deleting it. So, with that deletion, I think we are in fine shape. If someone else has feature requests it is fine to put them in as issues.

@paulray
Copy link
Member

paulray commented Feb 24, 2020

Also, note that other higher-level manipulations of event data that make use of PINT are in packages such as NICERSoft and HENDRICS. This include TOA generation and pulsation searches.

@luojing1211
Copy link
Member

Let us remove it in a separate PR, so we can better track on it.

@rossjjennings
Copy link
Member Author

OK, I took the removal out of this PR.

@paulray
Copy link
Member

paulray commented Feb 24, 2020

I guess we need to fix this conflict now that #618 has been merged.

@rossjjennings
Copy link
Member Author

rossjjennings commented Feb 24, 2020

I just rebased on master, skipping one commit that only modified htest_optimize.py. That should take care of it.

@rossjjennings
Copy link
Member Author

The Python 2.7 build is now failing for a reason I don't understand. One of the precision tests failed -- it looks like it might just have randomly chosen a rare example that it happens to break on?

@paulray
Copy link
Member

paulray commented Feb 24, 2020

Yeah, I don't understand it either. @aarchiba wrote those tests, so maybe she can help?
It says it is Flaky, but maybe one of the runs just timed out (as it tried to download an external file or something)? It suggests setting deadline=none but I don't know where that would be done.

@paulray
Copy link
Member

paulray commented Feb 24, 2020

Or sometimes if you just re-run the test it passes!

@rossjjennings
Copy link
Member Author

Just made a do-nothing change to re-trigger the Travis build.

@rossjjennings
Copy link
Member Author

OK, still failing. So it's not that Flaky...

@rossjjennings
Copy link
Member Author

@paulray Did you manage to reproduce it locally? I don't have a working Python 2.7 environment with PINT installed.

@paulray
Copy link
Member

paulray commented Feb 25, 2020

So, I just installed a py27 virtualenv and tried it two ways, with "make test" and with "tox -e py27". I noticed a couple of errors/warnings in the install:

/Users/psray/src/PINT/.tox/py27/lib/python2.7/site-packages/hypothesis/extra/pytestplugin.py:61: HypothesisWarning: 
        You are using Pytest version 3.6.4.  Hypothesis tests work with any test
        runner, but our Pytest plugin requires Pytest  4.3 or newer.
        Note that the Pytest developers no longer support this version either!
        Disabling the Hypothesis pytest plugin...
    
  warnings.warn(PYTEST_TOO_OLD_MESSAGE % (pytest.__version__,), HypothesisWarning)

and

ERROR: astropy 2.0.16 has requirement pytest<3.7,>=2.8, but you'll have pytest 4.6.9 which is incompatible.

Then, the "make test" passed the precision tests but had one fail in test_observatory:

../test_observatory.py:90: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../../../env/pint27/lib/python2.7/site-packages/pint/observatory/topo_obs.py:227: in clock_corrections
    clock_file, format=self.clock_fmt, obscode=self.tempo_code
../../../../env/pint27/lib/python2.7/site-packages/pint/observatory/clock_file.py:61: in read
    return cls._formats[format](filename, **kwargs)
../../../../env/pint27/lib/python2.7/site-packages/pint/observatory/clock_file.py:103: in __init__
    mjd, clk, self.header = self.load_tempo2_clock_file(filename)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

filename = '/Users/psray/src/tempo2-paulray/T2runtime/clock/fake2gps.clk'

    @staticmethod
    def load_tempo2_clock_file(filename):
        """Reads a tempo2-format clock file.  Returns three values:
        (mjd, clk, hdrline).  The first two are float arrays of MJD and
        clock corrections (seconds).  hdrline is the first line of the file
        that specifies the two clock scales connected by the file."""
>       f = open(filename, "r")
E       IOError: [Errno 2] No such file or directory: '/Users/psray/src/tempo2-paulray/T2runtime/clock/fake2gps.clk'

This is due to me having TEMPO2 installed, but I'm not sure why we have a test that fails looking for fake2gps.clk.

The tox run passed all tests.

So, I can't replicate the error either way. It may be due to the fact that hypothesis is being disabled. We may want to tweak the versions of packages being installed under py27.

@paulray
Copy link
Member

paulray commented Feb 25, 2020

Ah, looking at the test_observatory fail, it looks like it is supposed to fail, but the test is looking for OSError and it is triggering IOError instead. Perhaps something having to do with me having Tempo2 installed is causing the difference.

@paulray
Copy link
Member

paulray commented Feb 25, 2020

OK, I fixed test_observatory in my branch so it will test for both OSError and IOError.
That will remove the one issue.
But, I couldn't quite figure out a set of versions of hypothesis, pytest, astropy that all play happily together. astropy 2 wants an early version of pytest, while hypothesis wants a very recent version. This makes it difficult to have consistent tests on py27.

@paulray
Copy link
Member

paulray commented Feb 28, 2020

Should we merge this? I wonder if for the time being we should restore the astropy<4.0 specifications? Or are we really ready to allow astropy 4?

@rossjjennings
Copy link
Member Author

I finally got a chance to try to reproduce the test failure here in a working Python 2.7 environment today, and I can't. All the tests pass on this branch on my machine. Examining the Travis output more closely leads me to believe test_time_from_mjd_string_accuracy_vs_longdouble() was failing to meet its hypothesis-imposed deadline because astropy was fetching something over the network when it was first called, and doing that took longer than expected.

@paulray
Copy link
Member

paulray commented Feb 28, 2020

I concur. That must be the issue. Any idea how to extend the deadline?

@rossjjennings
Copy link
Member Author

We could turn off the deadline on this particular function by applying the @settings(deadline=None) decorator from hypothesis. It might not just be this function, though.

@paulray
Copy link
Member

paulray commented Feb 28, 2020

Seems like it is worth a shot

@rossjjennings
Copy link
Member Author

The relevant part of the hypothesis documentation is here.

Also, in response to your earlier comment, removing the astropy < 4 requirement is part of this PR, so it hasn't made it to the main branch yet.

@rossjjennings
Copy link
Member Author

I've added a commit extending the deadline to 1 second (from 200 milliseconds). Hopefully that fixes it. I'm still not sure why the issue only affects this branch, and only after I rebased. It can't be an astropy 4.0 thing, since it only happens on Python 2.7.

@rossjjennings
Copy link
Member Author

On whether we're really ready to allow astropy 4.0, I'd like to point out that all the Python 3 Travis builds of this branch are already using it, and have been passing for quite a while (even as Python 2.7 failed).

@rossjjennings
Copy link
Member Author

The extended deadline worked! All tests are now passing!

@paulray
Copy link
Member

paulray commented Feb 28, 2020

Sweet! I guess we can just take the plunge and enable astropy 4.0. We can always back off if we find an issue.

@scottransom
Copy link
Member

Yeah. Let's take the plunge and allow Astropy4 and merge this.

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.

4 participants