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

DM-39105: Write TMA state machine and event generator #52

Merged
merged 130 commits into from
Jul 19, 2023

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

@mfisherlevine mfisherlevine force-pushed the tickets/DM-39105 branch 3 times, most recently from 82bdcd1 to 05aa377 Compare June 28, 2023 15:18
Copy link

@r-owen r-owen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is as far as I got today. I'll do more tomorrow if I have time.

python/lsst/summit/utils/efdUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/efdUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/efdUtils.py Show resolved Hide resolved
python/lsst/summit/utils/efdUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/efdUtils.py Outdated Show resolved Hide resolved
# until a new event starts
if eventStart is None and state in skipStates:
rowNum += 1
continue
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mix of continue and loops within loops looks a bit complicated to me. Consider something like the following:

previousState = None
for rowNum, state in enumerate(states):
    if state in skipStates:
        previousState = None
        continue
    if state != previousState:
        eventStart = rowNum
        previousState = state
    parsedStates.append((...))
if previousState is not None:
    self.log.warning("Reached the end...")

Though I think your code appends the ending skipState to parsedStates. If you really want to do that, tweak the "if state in skipStates" a bit, e.g.:

if state in skipStates:
    if previousState is not None:
        parsedStates.append(...)
        previousState = None
    continue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've tinkered with this for a little while now and I can't seem to make this work quite the same way - something to do with needing to move through the event (which can have many states which are all the same, and should only end once the state changes). I agree the code I've written is pretty ugly, but all the alternative ways I've tried have come out just as ugly so far, so I think I might just have to leave it as-is for now 😔

python/lsst/summit/utils/tmaUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/tmaUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/tmaUtils.py Show resolved Hide resolved
python/lsst/summit/utils/tmaUtils.py Outdated Show resolved Hide resolved
@mfisherlevine mfisherlevine force-pushed the tickets/DM-39105 branch 23 times, most recently from 4919ae0 to b9b564d Compare July 6, 2023 22:56
Copy link
Contributor

@isullivan isullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it through everything but the unit tests, which I skimmed and didn't feel I had constructive comments about. I did not comment on code that I found Russell had already commented on.

python/lsst/summit/utils/efdUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/efdUtils.py Show resolved Hide resolved
python/lsst/summit/utils/efdUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/efdUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/efdUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/tmaUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/tmaUtils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/tmaUtils.py Show resolved Hide resolved
Comment on lines +1528 to +1657
assert getDayObsStartTime(dayObs) <= time
assert getDayObsEndTime(dayObs) > time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps raise a RuntimeError inside an if statement instead of bare asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets a little philosophical, but in my head at least, assert statements are used for things which should logically/definitionally never be possible, and are used to indicate that in code (i.e. sanity/consistency checks which should never fail, ever, and if they do, some function isn't doing what you thought it did). ifs and raises in places like that, - again, at least in my mind - are for things which one needs to check, defend against, and raise on when found, so this was why I chose naked asserts here rather than check & raise. (Also, I think naked asserts, for this reason, can be dropped at runtime, whereas if/raises cannot).

python/lsst/summit/utils/tmaUtils.py Outdated Show resolved Hide resolved
@psferguson
Copy link

clipDataToEvent does not currently appear in the __all__ of efdUtils.py

@mfisherlevine mfisherlevine merged commit 94c937e into main Jul 19, 2023
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.

4 participants