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 bug in eventstats and add test #587

Merged
merged 19 commits into from
Feb 22, 2020
Merged

Fix bug in eventstats and add test #587

merged 19 commits into from
Feb 22, 2020

Conversation

paulray
Copy link
Member

@paulray paulray commented Dec 12, 2019

This fixes a bug in sig2sigma().

I added a test, but it is not great, and really we should add tests for many of the other routines in this file.

@paulray
Copy link
Member Author

paulray commented Dec 16, 2019

@aarchiba @luojing1211 These test failures are in either test_ell1h.py or test_fbx.py and seem to occur randomly. They have nothing to do with my changes. Somehow those tests are extremely fragile. Any idea what is going on?

@luojing1211
Copy link
Member

This is because the relative differences between the analytical derivative and numerical derivative are a little bit bigger than the old tolerance (0.00104 vs 0.001). I found this is interesting, py2.7 does not report the bug.

@paulray
Copy link
Member Author

paulray commented Dec 16, 2019

So, do we increase the tolerance or fix the algorithm so they match more closely?

@luojing1211
Copy link
Member

I think we can do it now before we have a better way to test the derivative precision.

@paulray
Copy link
Member Author

paulray commented Dec 27, 2019

@luojing1211 when you say "I think we can do it now", what is "it"? I'd like to not have those failing tests.

@paulray
Copy link
Member Author

paulray commented Dec 30, 2019

This PR now includes fixes for using pulse numbers either from a par file or computed internally by PINT. I know I should have kept them separate.. sorry.

@paulray
Copy link
Member Author

paulray commented Dec 30, 2019

As soon as issue #593 is fixed, I'll add a good unit test for pulse numbers, which I think was not there before.

* nanograv/master:
  Blacken/isort
  Add unit test for models.Spindown.change_pepoch()
  Allow change_pepoch() to be used with toas=None
  Fix parfile writing
  Fix test_times
  fix erfautils
  Change numerical derivative step size for H3
  Increase stepsize
  Fixed import bug for maskParameters
  Fixed import and path issue with examples
* nanograv/master:
  Blackening of previous PR
* nanograv/master:
  Replaced bad nuppi clock file with correct Nancay clock file
* nanograv/master:
  Work around np.longdouble bug.
  black new test file
  add mulitiple observatories test.
  Extract proper motion in an astropy 2 compatible way
  add more tests for TOAs building
  Fix astropy table time column lost location information issue
  Get change_posepoch() to pass test
  Add test for change_posepoch()
  Make model a fixture in test_change_pepoch()
  Fix broadcasting corner case
  Fix broadcasting of get_psr_coords() over epoch
  Add change_pepoch()
  Set ECL=IERS2010 in test par file so test passes
  Return velocities from get_psr_coords()
  Allow 'pm_lat' and 'pm_lon_coslat' keywords in PulsarEcliptic under astropy 2
  Add test for ecliptic to ICRS conversion
@paulray
Copy link
Member Author

paulray commented Feb 22, 2020

OK, I'm going to merge this so that master will be passing all the tests.
The ex_pn.py in examples shows that pulse numbering is working but currently crashes due to issue #593
I'll try to be better about making smaller PRs going forward.

@paulray paulray merged commit ce095af into nanograv:master Feb 22, 2020
@paulray paulray mentioned this pull request Feb 22, 2020
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.

2 participants