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.

Sign up for GitHub

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

Jump to bottom

testing job checks #198

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

testing job checks #198

wants to merge 19 commits into from

Conversation

SallyElHajjar
Copy link
Collaborator

testing job checks

Katsutoshii
Katsutoshii requested changes Jan 21, 2025
View reviewed changes
Copy link
Collaborator

@Katsutoshii Katsutoshii left a comment

Choose a reason for hiding this comment

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

At a high level, a lot of this seems like experimental code that shouldn't be checked in. Can we start by checking in the dataset changes only?

usl_models/notebooks/train_atmo_model copy.ipynb Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove the _copy notebooks unless they are generally useful for future work. If we do keep them, give them a more meaningful name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okkay

usl_models/tests/atmo_ml/dataset_test.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of commenting out the entire file, can you put an early return on each of the failing tests?

usl_models/usl_models/atmo_ml/constants.py Outdated
# Geospatial constants
MAP_HEIGHT = 200
MAP_WIDTH = 200
# Geospatial constants should be 200
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we check this in? I thought this was for experimental purposes only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was thinking about that, but thought if I keep 200 with the new model architecture i will get the memory error again because the model will not be built. I can try do that

usl_models/training_log.txt Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add this to the gitignore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ohh i forgot to delete it. will remove it because we are no longer using it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Katsutoshii Katsutoshii Katsutoshii requested changes

@jainrajan98 jainrajan98 Awaiting requested review from jainrajan98

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SallyElHajjar @Katsutoshii