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

Use base 2 to report memory values in the table-humanreadable similar… #1018

Merged

Conversation

phupe
Copy link
Contributor

@phupe phupe commented Feb 5, 2019

In the memplot form the report, memory usage are plotted using a base 2 unit.

In the table-humanreadable from the report, the task table displays the memory using a base 10 unit.

The pull request harmonizes the memory unit such that base 2 unit is used in both cases.

…ly to the memplot

Signed-off-by: Philippe Hupé <philippe.hupe@curie.fr>
@@ -166,6 +166,7 @@ $(function() {
if (bytes == '-' || bytes == 0){
return bytes;
}
bytes = norm_mem([bytes]);
Copy link
Member

Choose a reason for hiding this comment

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

Does this change work? bytes is expected to be a number instead norm_mem returns an array.

@@ -166,6 +166,7 @@ $(function() {
if (bytes == '-' || bytes == 0){
return bytes;
}
bytes = norm_mem([bytes]);
// https://stackoverflow.com/a/14919494
var thresh = 1000;
Copy link
Member

Choose a reason for hiding this comment

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

Also, using thresh = 1024 should not do the same instead of using norm_mem ?

…ve base 2 unit memory in the Tasks table (nextflow-io#1018)

Signed-off-by: Philippe Hupé <philippe.hupe@curie.fr>
@phupe
Copy link
Contributor Author

phupe commented Feb 6, 2019

Using norm_mem surprinsingly worked out.

This is very true: much better to set thresh = 1024. This is done.

@pditommaso
Copy link
Member

Excellent!

@pditommaso pditommaso merged commit 0142609 into nextflow-io:master Feb 6, 2019
@phupe phupe deleted the harmonize_mem_unit_tasktable.vs.memplot branch February 6, 2019 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants