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

Resource allocation colormap #3453

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

Gossty
Copy link
Contributor

@Gossty Gossty commented Jan 7, 2025

No description provided.

@coveralls
Copy link

coveralls commented Jan 7, 2025

Coverage Status

coverage: 92.392% (+0.03%) from 92.362%
when pulling 60ec9fd on Gossty:resource-allocation-colormap
into ede77b4 on qiita-spots:dev.

Copy link
Member

@antgonza antgonza left a 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; do you think we can move the equations to the database in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this file? Can it be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can delete this file.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thank you; then please rm.

@Gossty
Copy link
Contributor Author

Gossty commented Jan 7, 2025

Looks good to me; do you think we can move the equations to the database in this PR?

We could do that. I think the best approach would be to have a function that would convert Python expression (e.g. np.log(x) * k...) into a string and upload it to DB similar to resource allocation table.

@antgonza
Copy link
Member

antgonza commented Jan 7, 2025

To be honest I think a direct insert would be easier - at the end, only admins with DB access can add them.

Copy link
Member

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

A few comments.

qiita_db/util.py Outdated

return fig, axs


def _retrieve_equations():
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this public by removing the _?

qiita_db/util.py Outdated
Comment on lines 2404 to 2409
for models in res:
if 'mem' in models[1]:
memory_models[models[1]] = lambda x, k, a, b: eval(models[2])
else:
time_models[models[1]] = lambda x, k, a, b: eval(models[2])
return (memory_models, time_models)
Copy link
Member

Choose a reason for hiding this comment

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

This loop can be outside the TRN, right?

qiita_db/util.py Outdated
+ np.log(x) * k)

MODELS_TIME = [time_model1, time_model2, time_model3, time_model4]


def get_model_name(model):
Copy link
Member

Choose a reason for hiding this comment

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

This method can really be replaced by _retrieve_equations, no? I mean, I don't understand why we need to have this here vs. using _retrieve_equations.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thank you; then please rm.

@@ -62,3 +62,11 @@ CREATE INDEX IF NOT EXISTS processing_job_command_parameters_payload ON qiita.pr
-- Addding contraints for the slurm_reservation column
ALTER TABLE qiita.analysis DROP CONSTRAINT IF EXISTS analysis_slurm_reservation_valid_chars;
ALTER TABLE qiita.analysis ADD CONSTRAINT analysis_slurm_reservation_valid_chars CHECK ( slurm_reservation ~ '^[a-zA-Z0-9_]*$' );

Copy link
Member

Choose a reason for hiding this comment

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

Note that we did a release this morning so these lines need to be moved to 94.sql.

@@ -0,0 +1,10 @@
INSERT INTO qiita.allocation_equations(equation_name, expression)
Copy link
Member

Choose a reason for hiding this comment

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

Same as the other sql: these lines need to be moved to 94.sql.

""")
self.assertEqual(failures, 1, "Number of failures must be 1")
self.assertEqual(failures, 0, "Number of failures must be 0")
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering why this changed from 1 to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the code chooses 4th model instead of 1st, which has 0 failures.

qiita_db/util.py Outdated
Comment on lines 2384 to 2406
for models in res:
model_name = "Unknown model"
if models[1] == 'mem_model1':
model_name = "k * log(x) + x * a + b"
elif models[1] == 'mem_model2':
model_name = "k * log(x) + b * log(x)^2 + a"
elif models[1] == 'mem_model3':
model_name = "k * log(x) + b * log(x)^2 + a * log(x)^3"
elif models[1] == 'mem_model4':
model_name = "k * log(x) + b * log(x)^2 + a * log(x)^2.5"
elif models[1] == 'time_model1':
model_name = "a + b + log(x) * k"
elif models[1] == 'time_model2':
model_name = "a + b * x + log(x) * k"
elif models[1] == 'time_model3':
model_name = "a + b * log(x)^2 + log(x) * k"
elif models[1] == 'time_model4':
model_name = "a * log(x)^3 + b * log(x)^2 + log(x) * k"
if 'mem' in models[1]:
memory_models[models[1]] = {
"equation_name": model_name,
"equation": lambda x, k, a, b: eval(models[2])
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? I mean the model_name is the expression column in the database, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have this for readability on the frontend. It was hard to read "np.log10(x)...". Maybe there's a way to automatically convert those from lambda function to a readable equation?

Copy link
Member

Choose a reason for hiding this comment

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

I see, I think leaving as np.log or doing just log in the database will be fine. Note that if you replace np.log for log in the database you would need to change the import numpy as np to from numpy import log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We previously also talked about adding parenthesis to ensure correct order of operations between systems, which makes the readability worse. Should we still keep the parenthesis or should we assume the regular order of operations? For example:
(k * (np.log(x))) + (b * ((np.log(x))**2)) + (a * ((np.log(x))**3))

Could just be k * log(x) + b * log(x)**2 + a * log(x)**3

Copy link
Member

Choose a reason for hiding this comment

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

I like the extra parenthesis so we don't assume the parenthesis order.

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.

3 participants