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

Additional frequency names #227

Merged
merged 4 commits into from
Dec 12, 2023
Merged

Additional frequency names #227

merged 4 commits into from
Dec 12, 2023

Conversation

GeorgeEfstathiadis
Copy link
Collaborator

Adding more Frequency names for better readability.

@GeorgeEfstathiadis GeorgeEfstathiadis added the enhancement New feature or request label Nov 29, 2023
Copy link
Member

@hackdna hackdna left a comment

Choose a reason for hiding this comment

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

I agree that this is cleaner but why do these need to be additional instead of replacement?

@GeorgeEfstathiadis
Copy link
Collaborator Author

Discussed this with JP, and agreed it's better this way because we have studies running their pipelines using forest. If we change this we would have to send out an alert for this to all study teams using this, which is not something we want to do right now, thus we only add the cleaner version on top of the other and just change MINUTELY to MINUTE.

@hackdna
Copy link
Member

hackdna commented Nov 29, 2023

The code in the develop branch is under active development by definition and is not meant to be used in production because all the code in it is subject to change. We decided to use this particular branching model to avoid this type of situation.

How and where are these studies running? Why are they not using the stable main branch? The teams should be able to avoid updating their version of Forest until the end of their studies so that we could continue with development?

@GeorgeEfstathiadis
Copy link
Collaborator Author

I think it's because oak is not part of the main release, there are a few teams that are working with accelerometer data and we have them install the develop version. Thus they are using that to run all of their data modalities.

I think for this it still makes sense to just add more options on top, because either HOURLY or HOUR etc. makes sense. But I agree that maybe we should discuss updating main at some point, so that develop can become again a more free change zone.

@biblicabeebli biblicabeebli mentioned this pull request Nov 30, 2023
@biblicabeebli
Copy link
Member

I just added pull request #228 and I believe the code in my branch is better suited than existing code for adding additional frequency options. The tl;dr is the 4 updated main functions for Willow, Sycamore, and Jasmine now iterate over a list of frequencies, which also stcks output into well-named folders.

The current code scrap I used is

if submits_timeframe == Frequency.HOURLY_AND_DAILY:
        submits_timeframes = [Frequency.HOURLY, Frequency.DAILY]
    else:
        submits_timeframes = [submits_timeframe]

We can easily write a function that decomposes any of the multiple time options into their component list.

@hackdna
Copy link
Member

hackdna commented Dec 1, 2023

@GeorgeEfstathiadis I see. We need to be careful with what we put in production so that we can maintain development velocity and avoid unnecessary technical debt. I think JP, you, and I (and @biblicabeebli if interested) should chat about this.

For this PR lets just rename MINUTELY to avoid confusion by adding duplicate names. We will rename the rest when it's safe to change the API.

Copy link
Member

@hackdna hackdna left a comment

Choose a reason for hiding this comment

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

Thank you!

@hackdna hackdna merged commit 416cdef into develop Dec 12, 2023
4 checks passed
@hackdna hackdna deleted the frequency_namings branch December 12, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants