-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
L3DAS22 enhancement recipe #4269
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4269 +/- ##
==========================================
- Coverage 82.80% 82.77% -0.04%
==========================================
Files 463 474 +11
Lines 40608 40755 +147
==========================================
+ Hits 33627 33733 +106
- Misses 6981 7022 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I just made some minor comments.
Could you also add your recipe to egs2/README.md? |
@@ -0,0 +1,61 @@ | |||
optim: adam |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very good to have multiple configs, could you also add results (dev would be fine) in RESTULS.md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This recipe should have a special evaluation (scoring) script prepared by an organizer (or by making the exact same scoring scheme by ourselves).
Thanks for the review and comments! I have addressed the comments and made two major changes to the PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very cool! some minor suggestion
Thanks for the comments! I have added the downloading data part in the recipe! |
@neillu23, some comments still exist, including the results. |
Thanks for reminding me! I haven't changed the data/temp to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. I think we can merge it after existing comments are addressed.
LGTM! I will merge it. Thanks a lot @neillu23 |
No description provided.