Skip to content

🚀 feat(tests) add time_series named 'a/b' in sample tests #91

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

casenave
Copy link
Member

Closes #82

@casenave casenave requested a review from a team as a code owner June 25, 2025 07:36
@casenave casenave requested a review from xroynard June 25, 2025 07:37
Copy link

codecov bot commented Jun 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

bstaber
bstaber previously approved these changes Jun 25, 2025
Copy link
Contributor

@bstaber bstaber left a comment

Choose a reason for hiding this comment

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

Great work

@xroynard
Copy link
Contributor

Do you see the problem now ?

@xroynard xroynard changed the title 🚀 feat(tests) add scalar named 'a/b' in sample tests 🚀 feat(tests) add time_series named 'a/b' in sample tests Jun 25, 2025
@bstaber bstaber dismissed their stale review June 25, 2025 18:34

New test shows that there is an issue

@bstaber bstaber self-requested a review June 25, 2025 18:34
@casenave
Copy link
Member Author

Can't we just forbid a "/" in the name of a time series (raises an error when trying to add one to a sample, for instance ?)

@casenave casenave marked this pull request as draft June 30, 2025 17:41
@casenave casenave added this to the version 0.1.7 milestone Jun 30, 2025
@xroynard
Copy link
Contributor

xroynard commented Jul 1, 2025

in the future, time_series might not be stored in files named by the time_series name, see #96

I would rather update the disk format to avoid using time_series name in filenames, as it is the case for other features (scalars, meshes, fields...), but it implies to respect #14,

@casenave Should this be done for Milestone v0.2.0 ?

As a workaround, for Milestone v0.1.7, I would rather implement something like:

  • before saving
time_series_name = time_series_name.replace('/', '\\')
  • after loading
time_series_name = time_series_name.replace('\\', '/')

@casenave
Copy link
Member Author

casenave commented Jul 2, 2025

@xroynard I think the time series are not really used for the moment, so ok for the renaming hack. Concerning retrocompatibility, it is important but I think we can impose it after a v1.0 version and allow us major modifications in the data model before - which would imply updating our public datasets

@xroynard
Copy link
Contributor

xroynard commented Jul 3, 2025

Can't we just forbid a "/" in the name of a time series (raises an error when trying to add one to a sample, for instance ?)

Maybe you were rigth, see https://cgns.org/standard/SIDS/convention.html

It is specified:

Names of entities and types are constructed using conventions typical of Mathematica [Mathematica 3.0, Wolfram Research, Inc., Champaign, IL (1996)]. Names or identifiers contain no spaces and capitalization is used to distinguish individual words making up a name; names are case-sensitive. The character “/” should be avoided in names, as well as the names “.” and “..”, as these have special meaning when referencing elements of a structure entity. An entity name cannot exceed 32 characters.

@xroynard
Copy link
Contributor

xroynard commented Jul 3, 2025

We should suggest to replace any occurence of / in a name by a character, but I see that windows does not allow \ and a lot of others:
image

maybe _

@casenave
Copy link
Member Author

casenave commented Jul 3, 2025

We should suggest to replace any occurence of / in a name by a character, but I see that windows does not allow \ and a lot of others: image

maybe _

thanks for looking into this! _ is fine to me

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.

Saving a Sample with names containing / fails
4 participants