Check up on restrictions to signac view #926
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #926 +/- ##
==========================================
- Coverage 85.68% 85.68% -0.01%
==========================================
Files 20 20
Lines 3458 3457 -1
Branches 758 758
==========================================
- Hits 2963 2962 -1
Misses 335 335
Partials 160 160
|
cbkerr
requested review from
tcmoore3,
shihkual and
b-butler
and removed request for
a team
b-butler
requested changes
May 31, 2023
signac/linked_view.py
Outdated
| @@ -64,7 +64,7 @@ def create_linked_view(project, prefix=None, job_ids=None, path=None): | |||
| key_list = [k for job in jobs for k in job.statepoint().keys()] | |||
| value_list = [v for job in jobs for v in job.statepoint().values()] | |||
| item_list = key_list + value_list | |||
| bad_chars = [os.sep, " ", "*"] | |||
| bad_chars = [os.sep] | |||
There was a problem hiding this comment.
We don't need a list any more. It should also be a quicker check to use ==.
|
Spaces were probably excluded because they don't work on older filesystems, and I'm guessing that one of the supercomputers we originally cared about (maybe the original Stampede, not even Stampede2? Not sure) was an example. I'm fine with removing the restriction and leaving the onus on the user to recognize why a view fails if they have such a filesystem. If we wanted, we could add a warning if spaces are detected saying that it may not work and is filesystem-dependent. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
b-butler
approved these changes
Jul 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
I had a space in my state point and was surprised that I couldn't make a signac view. This fix worked for me on mac but I'll run the tests on Windows and Linux through GitHub.
Motivation and Context
The PR that created this restriction was initially a fix involving path separation characters but there was no reason given for also excluding spaces and asterisks. I was able to create views in tests with these characters on mac.
I tested this locally with my project that had spaces in the state point after this change, and it worked.
There are other characters we might want to exclude as well, like
?,[],{}. These characters might be annoying to escape on the terminal, but a strong use of view is to browse in a file browser.Perhaps we can emit a warning about special characters being used?
I think if the tests pass in creating views, then we should be good to create views with spaces and asterisks in the state point.
Checklist: