-
Notifications
You must be signed in to change notification settings - Fork 4
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
Allow CALC_ prefix as an alias for DP_ #227
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks simple enough to me.
Ska/engarchive/tests/test_fetch.py
Outdated
dat3 = fetch.Msid('pitch', '2020:001', '2020:002') | ||
for d1, d2 in ((dat1, dat2), (dat1, dat3)): | ||
assert np.all(d1.times == d2.times) | ||
assert np.all(d1.vals == d2.vals) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you care that the MSID changes? in other words:
d1.MSID == d2.MSID
is True as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that change in the MSID from what the user provides was annoying me. The problem is that fixing that requires handling the different prefixes at a lower level in the code. So in the end I went for "perfect is the enemy of good" here.
I think that if you provide a single MSID (i.e. not a glob) then d1.msid
will reflect what you provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no issues with it changing as long as one knows and perhaps documented, I just checked that it does change:
In [1]: from cheta import fetch
In [2]: dat1 = fetch.Msid('calc_pitch', '2020:001', '2020:002')
In [3]: dat1.msid
Out[3]: 'DP_pitch'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if one is going to change it, perhaps it should be "normalized", meaning that the name is always the same for a given msid.
@javierggt - I think I addressed your concerns and made this a little better. The input msid is always left unchanged (when there is no fileglob |
@javierggt @jeanconn - ping that this is ready for review and I addressed the initial comments. |
Description
MAUDE uses the prefix
CALC_
to indicate pseudo-MSIDs that are calculated from actual spacecraft MSIDs, for exampleCALC_ROLL_FSS
. Cheta usesDP_
for the same meaning, and many calcs have the same name apart from CALC_ vs DP_. The good news is that MAUDE also acceptsDP_
as a prefix.This PR does a simple translation of
CALC_
toDP_
in input MSIDs to enable interoperability. This is especially useful for a query that uses both CXC and MAUDE for the backend data source. This translation is seen inself.MSID
(backend query version) but not inself.msid
(user-supplied version).Finally, the
plot
method was updated to use an upper-cased version of the user-supplied MSID. So if the user asks forcalc_pitch_fss
then they seeCALC_PITCH_FSS
in the plot title, whilepitch_fss
goes toPITCH_FSS
. In both cases the internalself.MSID
would beDP_PITCH_FSS
since that is what is used in the backend query.This requires https://github.com/sot/maude/pull/39
Interface impact
The interface now allows MSIDs with a prefix of
CALC_
.The title in plots made from an
MSID
object has been changed. Previously it includedself.MSID
which isthe internal version used to actually fetch telemetry data. Now it is
self.msid.upper()
, which is an upper-cased version of the MSID specified by the user in the query. For instance if you diddat = fetch.Msid('pitch_fss', ...)
, previously the plot title wasDP_PITCH_FSS
but now it will bePITCH_FSS
.Testing
Functional testing
Made the following plots to confirm the update to the plot title.