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

Barytime fix #650

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Barytime fix #650

wants to merge 9 commits into from

Conversation

s-silvestri
Copy link
Collaborator

A fix and a feature:

Feature
If the input file has a BARYTIME column that is used by default, otherwise TIME is used as previously

Fix
The met0 is used for phase folding instead of TSTART

@lucabaldini
Copy link
Owner

Ciao Stefano,
we have the unit tests failing on both Linux and Mac---I am running the unit tests locally now to try and diagnose that.

@lucabaldini lucabaldini self-assigned this Oct 8, 2022
@lucabaldini
Copy link
Owner

I am looking at the change right now. Instead of encapsulating the logic for choosing the time column, how about adding a command-line argument (e.g., --timecol), defaulting to TIME, that one can set to BARYTIME?

This would seem like a more flexible mechanism, and would diminish complexity in the inner part of the code.

@lucabaldini
Copy link
Owner

Here is the error from the unit tests---we have broken something.

======================================================================
ERROR: test_xpphase (test_xpphase.TestXpphase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/work/ixpe/ixpeobssim/tests/test_xpphase.py", line 64, in test_xpphase
    file_list = pipeline.xpphase(*file_list, parfile=self.file_path, suffix='folded')
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/core/pipeline.py", line 427, in xpphase
    return _xpphase(**_parse_args(XPPHASE_PARSER, *args, **kwargs))
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/bin/xpphase.py", line 118, in xpphase
    phase = eph.fold(time_, kwargs.get('met0'), kwargs.get('phi0'))
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/srcmodel/ephemeris.py", line 340, in fold
    nu0 = self.nu(start_met)
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/srcmodel/ephemeris.py", line 299, in nu
    dt = self._dt(met)
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/srcmodel/ephemeris.py", line 294, in _dt
    return met - self.met0
TypeError: unsupported operand type(s) for -: 'NoneType' and 'float'

======================================================================
ERROR: setUpClass (test_xpselect_inverse.TestSelect)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/work/ixpe/ixpeobssim/tests/test_xpselect_inverse.py", line 49, in setUpClass
    cls._file_list = pipeline.xpphase(*file_list, nu0=1.)
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/core/pipeline.py", line 427, in xpphase
    return _xpphase(**_parse_args(XPPHASE_PARSER, *args, **kwargs))
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/bin/xpphase.py", line 118, in xpphase
    phase = eph.fold(time_, kwargs.get('met0'), kwargs.get('phi0'))
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/srcmodel/ephemeris.py", line 340, in fold
    nu0 = self.nu(start_met)
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/srcmodel/ephemeris.py", line 299, in nu
    dt = self._dt(met)
  File "/data/work/ixpe/ixpeobssim/ixpeobssim/srcmodel/ephemeris.py", line 294, in _dt
    return met - self.met0
TypeError: unsupported operand type(s) for -: 'NoneType' and 'float'

----------------------------------------------------------------------
Ran 353 tests in 583.289s

@s-silvestri
Copy link
Collaborator Author

It can be done. Although i don't see why anyone would use the non barycentered TIME column if he has BARYTIME

@lucabaldini
Copy link
Owner

Ok, the issue is that --met0 defaults to None, and in that cas the subtraction fails. We need a better default---how about 0, i.e., the start of the mission?

@lucabaldini
Copy link
Owner

Stefano, let's move this discussion on the issue #651

I am not sure anymore we're doing the right thing :-)

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