-
Notifications
You must be signed in to change notification settings - Fork 6
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
Align to latest Nextflow version and bump to groovy 4.0 #91
Align to latest Nextflow version and bump to groovy 4.0 #91
Conversation
cb1518a
to
ad09eb2
Compare
@@ -182,7 +182,7 @@ class CO2FootprintFactory implements TraceObserverFactory { | |||
* Factors of memory power usage | |||
*/ | |||
// nm: size of memory available [GB] -> requested memory | |||
Long memory = trace.get('memory') as Long |
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.
Why did you change this? I think Long
would be needed here
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.
The function returns a List<Double>
, and within this list the memory. So to return it actually as said list the memory needs to be cast to double. If the memory is returned as memory, the function would need to be set to return something like List<numerical>
which in turn would need casting to doubles again. So in this case I would say the lesser evil is to cast the memory to double, even though it is a non float number.
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.
ok, I don't get the exact problem. Here it's also accessed like this: https://github.com/nextflow-io/nextflow/blob/949181053afce9e66ebde1ccff3c725fb2f61697/modules/nextflow/src/main/groovy/nextflow/trace/ReportSummary.groovy#L52
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 was about this list:
nf-co2footprint/plugins/nf-co2footprint/src/main/nextflow/co2footprint/CO2FootprintFactory.groovy
Line 219 in c01bbde
return [e, c, realtime, nc, pc, uc, memory] |
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.
conclusion: although long
would be more optimal, using double
should be sufficient to represent the memory values here
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.
Maybe we can convert the returned list
to a map
in a follow-up 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.
I added a comment and a todo to the cast, which is now later to calculate the nm
and respective used energy with the full precision memory
Signed-off-by: Till Englert <till.englert96@gmail.com>
Signed-off-by: Till Englert <till.englert96@gmail.com>
Signed-off-by: Till Englert <till.englert96@gmail.com>
Signed-off-by: Till Englert <till.englert96@gmail.com>
Signed-off-by: Till Englert <till.englert96@gmail.com>
Signed-off-by: Till Englert <till.englert96@gmail.com>
…full precision data. Also add a comment, why it is casted and add TODO Signed-off-by: Till Englert <till.englert96@gmail.com>
fb4f798
to
96db2e7
Compare
The force push was to sign off every commit using, which is now required. |
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!
This PR will align the plugin to the latest nextflow version (24.08.0-edge).
It will deprecate all nextflow versions prior to 24.01.0-edge.
(I might need to add some updates to the test workflows, but we'll see if that can be changed apart of this PR)