DM-39852: Have TMA events know their block numbers #56
Conversation
mfisherlevine
force-pushed
the
tickets/DM-39852
branch
2 times, most recently
from
b897b83
to
65ba2c5
Compare
mfisherlevine
force-pushed
the
tickets/DM-39852
branch
2 times, most recently
from
b116936
to
9f150ff
Compare
mfisherlevine
force-pushed
the
tickets/DM-39852
branch
from
9f150ff
to
830bfef
Compare
| import time | ||
| import logging | ||
| import pandas as pd | ||
| import numpy as np |
There was a problem hiding this comment.
Very minor point, but please alphabetize the imports.
There was a problem hiding this comment.
Soon I'll be adding pre-commit to this whole package I think, running isort and black on everything, so this will all be taken care of. I think this large ticket is actually the only blocker for doing that, so will just deal with this once this merges.
| salIndices : `list` of `int` | ||
| One or more SAL indices, relating to the block. | ||
| tickets : `list` of `str` | ||
| One or more SITCOM tickets, relating to the block. |
There was a problem hiding this comment.
Perhaps specify "Jira tickets"
There was a problem hiding this comment.
These are actually specifically SITCOM tickets (this is how they're parsed out). I guess at some point, if it's necessary, I could change the parsing and try to suck all Jira ticket formats out, but so far I think it's only ever SITCOM type tickets, and that certainly makes the data parsing a lot easier too. If I do ever change that I'll be sure to update these docs, but as they are, this is actually correct.
| class BlockParser: | ||
| """A class to parse BLOCK data from the EFD. | ||
|
|
||
| Information on executed blocks is stored in the EFD in the |
There was a problem hiding this comment.
Perhaps spell out the EFD acronym once in the description.
There was a problem hiding this comment.
Have done so (but just so you know, anyone who is anywhere near this stuff will definitely know what this means already, it'd be like working on the mountain and not knowing what TMA stands for).
|
|
||
| t0 = time.time() | ||
| self.getDataForDayObs() | ||
| self.log.debug(f"Getting data took {(time.time()-t0):.2f} seconds") |
There was a problem hiding this comment.
Probably not important here, but I have been discouraged from using f-strings in debug messages for performance reasons. Feel free to leave this as-is.
There was a problem hiding this comment.
Thanks - I think that's generally a very good point, but given this is only on init I think it's fine here, but in general I think that's an important point, yes.
| data : `pandas.DataFrame` | ||
| The row data. | ||
| """ | ||
| rowsForBlock = self.data[self.data['blockNum'] == block] |
There was a problem hiding this comment.
Perhaps add an error message if block is not in self.data['blockNum']
There was a problem hiding this comment.
Added a log warning (but it shouldn't be an error, I don't think).
| raise RuntimeError(f"Found multiple blockIds ({blockIds}) for {seqNum=}") | ||
| blockId = blockIds.pop() | ||
|
|
||
| blockInfo = BlockInfo( |
There was a problem hiding this comment.
Do you need to preserve the relation between specific salIndices and tickets from the same row of the block? If so, that has been lost here.
There was a problem hiding this comment.
I am almost certainly that I don't, no, but that's a good point, thanks!
|
|
||
| """Test cases for utils.""" | ||
|
|
||
| import os |
There was a problem hiding this comment.
Please alphabetize imports, and move the lsst.utils.tests import to the lsst section.
There was a problem hiding this comment.
Thanks, I've moved the import lsst.utils.tests section, and will wait for isort to fix the rest.
| data[f'{block}{DELIMITER}{seqNum}'] = line | ||
|
|
||
| packageDir = getPackageDir("summit_utils") | ||
| dataFilename = os.path.join(packageDir, "tests", "data", "blockInfoData.json") |
There was a problem hiding this comment.
It could be useful to provide dataFilename as a keyword argument, and default to this value if none is provided.
There was a problem hiding this comment.
Good shout, thanks.
| def setUpClass(cls): | ||
| try: | ||
| cls.client = makeEfdClient() | ||
| except RuntimeError: |
There was a problem hiding this comment.
Please add a comment describing when you would expect makeEfdClient to fail.
There was a problem hiding this comment.
That's a really good point, and I actually don't know when I would expect that. Maybe when running from some place that isn't one of our systems? But I think the more important point that you've made me realise here, is that I've not annotated this to use the vcr package here, so this will end up contacting an actual EFD, which is no good at all! I will get that part fixed up right now!
EFD queries have the actual query in the body of a POST request. If this isn't matched on in full then you get random data back. It is actually quite amazing that the tests were passing as well as they were. The bug-fix now means you actually always get the correct data back from vcr.
No description provided.