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

Avoid torchscript errors when float options are supplied as integers #419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

frostedoyster
Copy link
Collaborator

@frostedoyster frostedoyster commented Dec 9, 2024

Closes #414. Implements a fix for the case where float options are instead supplied as integers

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

📚 Documentation preview 📚: https://metatrain--419.org.readthedocs.build/en/419/

@frostedoyster
Copy link
Collaborator Author

frostedoyster commented Dec 9, 2024

@abmazitov I need your help for PET, because I don't know where the best place to convert the parameters to float is. We could do it here (although it's a bit of a hack) or in the PET repository, as you wish

@Luthaf
Copy link
Member

Luthaf commented Dec 9, 2024

Is this something we could do using the json schema validation? I think there is a way to specify that a value should be a float and not an integer there, and we could maybe use this information to ask the validator to accept both and then do the conversion ourself.

This mean we would not have to do this (and think about it) for each new architecture.

@frostedoyster
Copy link
Collaborator Author

There is a way to specify that a number should be a float, but then the validation will fail, right?
I implemented the fix as you and others agreed in the issue

@Luthaf
Copy link
Member

Luthaf commented Dec 9, 2024

I was thinking of something where (a) we declare that we want a float in the schema, but (b) we modify the schema before validation to also allow for integers and (c) we do the conversion after the validation. This way the user does not have to think about it, and the architecture developers do not have to think about it. But it might be a lot of work for minimal gains, so the current fix should also be fine!

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.

Some parameters might not work if given as integers
2 participants