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

improve and move isfloat(num) #127

Merged
merged 8 commits into from
Oct 8, 2024
Merged

Conversation

NormannK
Copy link
Collaborator

@NormannK NormannK commented Oct 7, 2024

fixing #126 by adding case "None", stings with whitespace and special types like +-inf.
Also moving the function to the server since its only used there.

function is not used here
improved isfloat for case "None" and strings with surrounding whitespace and special types like +-inf.
@@ -26,6 +27,25 @@
)


def isfloat(num: Any) -> bool:
Copy link
Contributor

@noootch noootch Oct 7, 2024

Choose a reason for hiding this comment

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

you could add a TypeGuard:

from typing import TypeGuard
def isfloat(num: Any) -> TypeGuard[float]

if you test if isfloat(my_var) my_var will be recognized as float by the linter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh that's a very nice function I did not know about.
According to PEP 647 thats in Version 3.10 introduced. Currently we need 3.8+ according to the readme. Would 3.10+ be a problem? Ill add it

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not check the required python version, my bad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you see any reason not to change the python version needed? 3.10 is already 4 years old.

Copy link
Contributor

Choose a reason for hiding this comment

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

personally I would even go to 11 or even 13. But as the program is also distributed as a package that other people can implement, it is better to support more versions.

@drbacke drbacke merged commit bda6d24 into Akkudoktor-EOS:main Oct 8, 2024
2 checks passed
@NormannK NormannK deleted the patch-2 branch October 8, 2024 08:11
@drbacke
Copy link
Collaborator

drbacke commented Oct 8, 2024

@NormannK None is not accepted by JSON.
The code does not work, maybe you should not change everything I have coded without understanding or being able to test it. I don't mean any offense, but I've had to change a lot of things back. A little more precaution would be really nice.
Also np.ndarray is not JSON serializable.

@NormannK
Copy link
Collaborator Author

NormannK commented Oct 8, 2024

@drbacke

I guess I'm the only one besides you running that code actively. In my backend I save everything into a table and go from there. Also I simply ignore the first value.

But i see what you are referring too. So we don't have proper type sanitation before translating it into json.
Since the Flask server can't call the EMS the only way to hand the result over must go through the optimization.
So the right way would be to translate everything which comes out of the optimization class into lists where the None value can be translated into the equivalent "Null" json value.
Im looking into the code of the flask_server for "gesamtlast_simple" we do a return jsonify(last.tolist())
But for "optimize" there is a simple return jsonify(result).
So the right way to do it take care of the translations at the last step where we do the conversion and not in between.
Also if that fails we miss a test case....

@noootch
Copy link
Contributor

noootch commented Oct 8, 2024

Pydantic is also a good option to parse data from and to json. The bonus point: you have a nice object that carries your data so you can access it via attributes which increases type safety.

I used it in my suggestion for the config.

@drbacke regarding the config, can you give me a feedback why you closed the PR?

@michaelosthege
Copy link
Collaborator

michaelosthege commented Oct 8, 2024

@noootch you are referring to #79. I really liked your proposal to use pydantic to data validation.

I'm 95 % sure it got closed automatically when @drbacke accidentally force-pushed the main branch to a new history (same commits, new hashes from day 0) 👇

image

@noootch
Copy link
Contributor

noootch commented Oct 8, 2024

meeeeh, ok. I'll reopen with the new branch.

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.

4 participants