-
Notifications
You must be signed in to change notification settings - Fork 25
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
TXTExport features #676
TXTExport features #676
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #676 +/- ##
==========================================
+ Coverage 98.92% 98.93% +0.01%
==========================================
Files 57 57
Lines 2130 2154 +24
==========================================
+ Hits 2107 2131 +24
Misses 23 23 ☔ View full report in Codecov by Sentry. |
This could be a good opportunity to add in |
@jhdark that's an excellent idea! We didn't have a file name initially since it was defined from the label (and we had several files per export) but now we can definitely have a file name argument! @KulaginVladimir do you think you could add this? |
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.
Hi @KulaginVladimir a few comments. Also as @jhdark said maybe we could get rid of label
and simply expose filename
just like we do for other exports.
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 looks great to me! Thanks @KulaginVladimir
I'll let @jhdark review this before merging since he suggested the filename
property
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 good to me
Proposed changes
This PR implements several minor modifications to
TXTExport
&TXTExports
classes:TXTExport
was modified to accept a user-defined format for column headers in the output files.TXTExports
&TXTExport
features #675:TXTExport.times
are passed toStepsize.milestones
(if they are not present) in order to ensure that data is actually exported at these times.TXTExports
was modified to allow users export data at all timesteps. Also, a deprication warning was added.Types of changes
What types of changes does your code introduce to FESTIM?
Checklist