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

Placeholder resolutions #568

Open
fcollman opened this issue Nov 20, 2022 · 12 comments
Open

Placeholder resolutions #568

fcollman opened this issue Nov 20, 2022 · 12 comments
Labels
feature A new capability is born. images Pertains to volumetric image representation, interfaces, or IO.

Comments

@fcollman
Copy link
Collaborator

When users try to go from neuroglancer>cloudvolume with flywire they are tempted to use naive code like this

im_cv= cloudvolume.CloudVolume('precomputed://https://bossdb-open-data.s3.amazonaws.com/flywire/fafbv14')

but this will give them a 4,4,40 mip which is blank, and no warnings as to what is going on.

the info file has a key= 'placeholder', but cloudvolume isn't trying to detect this to auto-avoid configuring its mip to that, or at a minimum warning the user that this might be a placeholder mip.

@william-silversmith
Copy link
Contributor

Hi Forrest, those datasets are pretty annoying, I make those mistakes all the time. I suspect auto-config to mip 1 will be non-intuitive. People will get confused why they are on mip 1. A warning could be a good idea, but would also need a way to suppress it to avoid corrupting logging.

I do think the key=placeholder is a bit of a hack (I wrote it years ago). Ideally, we'd add another attribute to avoid making prescriptions on key names (and to allow multiple scales to be placeholders).

@william-silversmith william-silversmith added feature A new capability is born. images Pertains to volumetric image representation, interfaces, or IO. labels Nov 21, 2022
@sdorkenw
Copy link
Member

Hi Will. I can confirm that this is a common issue that is very confusing to new but also seasoned users.

The situation is bad from a users perspective - the empty mip is a hack and is causing it. Being auto-configured to a mip > 0 is not great but so is not receiving any data when using the standard initialization. I would argue that the latter - not receiving any data (aka just 0s) - is the more confusing scenario. There, a user does not even know what the problem is and first suspects it must be something else.

I think auto-configuring to the first mip that has actually data is best from a users perspective. This appears to be a major change though because it can change the behavior for code that just uses the bounding boxes.

@william-silversmith
Copy link
Contributor

william-silversmith commented Nov 21, 2022

Yea, that's my biggest concern that code that really depends on mip 0 will get broken. Breaking existing code is not so good. Having users get confused really sucks too, but at least they'll fix it on the spot.... eventually and painfully.

Can someone remind me why we even have these placeholders? Was it to make alignment easier? Would it be possible to just drop them now?

@fcollman
Copy link
Collaborator Author

as a compromise can we raise a warning when someone auto-configures mip=0 and it has a key of placeholder, and suggest they might want to configure mip=1?

@sdorkenw
Copy link
Member

I think we did that [1] to keep segmentation and EM data the same in terms of resolutions and [2] to make it easier to render out a 4,4,40 mip (aka mip 0) later. Removing them works but can break things in the same way the code change would.

The warning flag seems like a good middle ground - at least it helps the user to figure out whats going on without breaking anything

@william-silversmith
Copy link
Contributor

Okay, a warning it is then. That seems to be the least offensive option. Thanks for the feedback!

@fcollman
Copy link
Collaborator Author

we could also add a deprecation warning that in the future we will do this automatically and then in the future raise a warning that we did it.

@william-silversmith
Copy link
Contributor

My feeling is let's see how annoying the warning is. It might not be so bad. I'm not quite ready to promise that we're going to put some magic in (that will probably never be able to be removed if it wasn't a good idea).

@william-silversmith
Copy link
Contributor

Check out this branch and let me know what you think.
https://github.com/seung-lab/cloud-volume/tree/wms_placeholder_warning

@fcollman
Copy link
Collaborator Author

Looks good.. as a note, Casey released some changes to imagery client today that lets auto-detects the placeholder resolutions and will avoid selecting them in order to simplify making overlaid images and segmentation.

this now works for flywire and produces the following image using imageryclient>=1.0.1

import imageryclient as ic
from caveclient import CAVEclient

client = CAVEclient('flywire_fafb_production')
img_client = ic.ImageryClient(client=client)

image, segs = img_client.image_and_segmentation_cutout(
    [143115, 73649, 3534],
    bbox_size=[1024, 1024],
    split_segmentations=True,
)

img = ic.composite_overlay(segs, imagery=image, palette='husl', alpha=0.3)

image

@fcollman
Copy link
Collaborator Author

warning looks good

@william-silversmith
Copy link
Contributor

I'll integrate this (#571) into the next release. Good idea Casey!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new capability is born. images Pertains to volumetric image representation, interfaces, or IO.
Projects
None yet
Development

No branches or pull requests

3 participants