Skip to content

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

Merged
mfisherlevine merged 130 commits intomainfrom
tickets/DM-39105
Jul 19, 2023
Merged

DM-39105: Write TMA state machine and event generator#52
mfisherlevine merged 130 commits intomainfrom
tickets/DM-39105

Conversation

@mfisherlevine
Copy link
Copy Markdown
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
Copy Markdown

@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.

# until a new event starts
if eventStart is None and state in skipStates:
rowNum += 1
continue
Copy link
Copy Markdown

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
Copy Markdown
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 😔

@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
Copy Markdown
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.

Comment on lines +1528 to +1657
assert getDayObsStartTime(dayObs) <= time
assert getDayObsEndTime(dayObs) > time
Copy link
Copy Markdown
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
Copy Markdown
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).

@psferguson
Copy link
Copy Markdown

clipDataToEvent does not currently appear in the __all__ of efdUtils.py
https://github.com/lsst-sitcom/summit_utils/blob/b9b564d8d4fb94c45e3362bc17dcb7ccc08d9073/python/lsst/summit/utils/efdUtils.py#L37

@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