search

sot / chandra_aca

Chandra Aspect Camera Tools
https://sot.github.io/chandra_aca
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

Bugfix: make sure maude_result argument to get_raw_aca_blobs is not modified inside the function #129

Closed javierggt closed 2 years ago

javierggt commented 2 years ago

Description

Unfortunately, I found yet another issue with blobs in maude_decom. This issue does not affect anything I have done up to now.

The maude_decom.get_raw_aca_blobs takes a maude_result argument, which contains the blobs as returned by maude, and this object is (inadvertently) modified inside the function. As a result, if one tries to use the blobs after calling this function, the data structure has changed and this causes errors. I would prefer to keep the argument unchanged, because whatever changes the function made are implementation details.

One could argue that the modified blobs are a better, because each blob's values are transformed from a list to a dictionary, which is better to fish out specific MSIDs. Namely, the master version does the following:

for b in maude_blobs['blobs']:
    b['values'] = {v['n']: v['v'] for v in b['values']}

although I think this is better as it does not loose information:

for b in maude_blobs['blobs']:
    b['values'] = {v['n']: v for v in b['values']}

If we prefer to modify the blobs, then the modification should be done in a separate function, perhaps in maude, and not as a side effect of any function, so I think this qualifies as a bug in any case.

Interface impacts

None

Testing

Unit tests

This PR has two commits. In the first commit I add a new test which fails and is fixed by the second commit.

Independent check of unit tests by [REVIEWER NAME]

Functional tests

No functional testing.

taldcroft commented 2 years ago

I think that a new helper function in maude which basically inverts the data structure to be keyed off of MSID names instead of lists (like you showed) would be a good thing. We might want to go further and turn it into a data structure that is roughly like a fetch.MSIDset(). At least to me that is convenient and follows a well-established protocol. :smile:

But this discussion is separate and out of scope for this PR, which looks fine to me.