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

add dummy file name for some tiffseq #592

Merged
merged 5 commits into from
Oct 26, 2023
Merged

Conversation

dylanmcreynolds
Copy link
Contributor

This addresses a bug with the tiff sequence code.

When the files of a tiffsequence don't start with letters, just numbers, a hash was used for the key for that node.

So:

tiff_files
  - 001.tiff
  - 002.tiff
  - a001.tiff
  - a002.tiff
  - b001.tiff
  - b002.tiff

would result in

['d3a7b120-91f8-41ef-a1ff-f3e312f23525',
 'a',
 'b']

The unit test wasn't expecting a random name for this node, so it didn't test for it.

This PR detects that case and adds an _ in as the key name. On a call with @taxe10 and @danielballan, we debated this default root name for only a couple seconds. I'm happy to hear better ideas.

@padraic-shafer
Copy link
Contributor

padraic-shafer commented Oct 26, 2023

This seems ok good. :)

Other thoughts, just to throw them out there: '', ' ', or None.

>>> d1 = {'': 0, 'a':1, 'b': 2}
>>> d1['']
0

>>> d2 = {' ': 0, 'a':1, 'b': 2}
>>> d2[' ']
0

>>> d3 = {None: 0, 'a':1, 'b': 2}
>>> d3[None]
0

# But not...
>>> d3[]
  File "<stdin>", line 1
    d3[]
       ^
SyntaxError: invalid syntax

@danielballan
Copy link
Member

Since the keys also end up in path parameters, this unfortunately rules out a couple of those options:

  • '' ends up as /api/v1/metadata/tif// (emphasis on //) which HTTP elides into /.
  • None has to to coerced to a string of some kind, so its special status as an out-of-band sentinel does not survive.
  • Space technically works but may invite confusion.

I think we'll be happy if we stick with "The key must be a non-empty string."

@danielballan
Copy link
Member

Feedback from @Wiebke, who thought about related issues in her work on #555, would be very welcome as well.

@dylanmcreynolds
Copy link
Contributor Author

I almost feel like something super explicit might be better, even if a bit verbose:

['_unnamed_sequence',
 'a',
 'b']

@padraic-shafer
Copy link
Contributor

padraic-shafer commented Oct 26, 2023

Explicit and brief would be even better: "_default", "_unnamed", "_blank", "_na", etc.

I favor including a non-alphanumeric to try to minimize name clashes with the user-defined sequences. Something that does not require url-encoding would be good. I suppose a leading "_" will list this key at the beginning for most sorting routines?

EDIT: Or if only letters are allowed for the sequence names, that would leave "0" (zero) available. Is that too cryptic? Or too easy to confuse ['0'] with [0] or ['O']?

@danielballan
Copy link
Member

👍 for brevity. Any of these seem good to me:

"_default", "_unnamed", "_blank", "_na", etc.

Using a number would get weird in this context:

c['tif']['0'][0]  # first image

@dylanmcreynolds
Copy link
Contributor Author

I committed _unnamed. I like it because it's pretty explicit.

Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

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

Looks good to me, but we should wait for @Wiebke to weigh in before we merge.

Copy link
Contributor

@Wiebke Wiebke left a comment

Choose a reason for hiding this comment

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

I think this is a sensible solution for unnamed sequences and I liked _unnamed as well.

@Wiebke Wiebke merged commit 1db4a8b into bluesky:main Oct 26, 2023
8 checks passed
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