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

Update parameters.py #1542

Merged
merged 6 commits into from
Jan 8, 2025
Merged

Update parameters.py #1542

merged 6 commits into from
Jan 8, 2025

Conversation

stephanmg
Copy link
Contributor

@stephanmg stephanmg commented Dec 18, 2024

Quick fix for issue #1535

Also underlying cause is that pypesto.store.read_result loads result.problem.x_scales as <class 'bytes'> and error surfaces here during string concatenation. (Maybe return attributes consistently as <class 'str'> throughout?)

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.34%. Comparing base (20fd9de) to head (e0998a0).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1542      +/-   ##
===========================================
- Coverage    84.36%   84.34%   -0.03%     
===========================================
  Files          163      163              
  Lines        14034    14035       +1     
===========================================
- Hits         11840    11838       -2     
- Misses        2194     2197       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@m-philipps
Copy link
Contributor

Thanks. Is there a reason for using bytes instead of str in pypesto.store.read_result? If not, I would be in favour of changing that.

@stephanmg
Copy link
Contributor Author

stephanmg commented Dec 18, 2024

Thanks. Is there a reason for using bytes instead of str in pypesto.store.read_result? If not, I would be in favour of changing that.

Not sure entirely - but in general byte array has a smaller storage footprint (str depends on encoding) - also to my understanding faster to process. (Don't think that matters there, but maybe for other attributes of the read-in result object). Maybe someone can shed light on that - then I agree, would make more sense to change it in pyesto.store.read_result accordingly.

@stephanmg
Copy link
Contributor Author

stephanmg commented Dec 18, 2024

pypesto.store.read_result reads into thepypesto.result, problem has the attribute x_scales with type hint defined as Optional[Iterable[str]]... still when I print directly result.problem.x_scales I get a byte str as reported by Clemens too.

the issue originates from pypesto.store.read_from_hdf5.py - ProblemHDF5Reader class.

data is read from the h5 file, which provides byte strings, but is never decoded (x_names is done correctly and decoded though)

a bit odd that this did not surface more often in the past?

Revert changes.
Decode byte str to str.
@stephanmg
Copy link
Contributor Author

stephanmg commented Dec 18, 2024

Now should be fixed at the point or origin. Please have a look, ping @PaulJonasJost

@stephanmg stephanmg self-assigned this Dec 18, 2024
Copy link
Collaborator

@PaulJonasJost PaulJonasJost left a comment

Choose a reason for hiding this comment

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

Thanks for noticing 👍🏼

@stephanmg
Copy link
Contributor Author

Thanks for noticing 👍🏼

All credits to @C-Peiter for reporting the issue ;-)

@stephanmg stephanmg added this pull request to the merge queue Jan 8, 2025
Merged via the queue into develop with commit 1121bcc Jan 8, 2025
18 checks passed
@Doresic Doresic mentioned this pull request Jan 8, 2025
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