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

Use meaningful domain type locations/imports #95

Open
SimonHeybrock opened this issue Feb 20, 2024 · 11 comments
Open

Use meaningful domain type locations/imports #95

SimonHeybrock opened this issue Feb 20, 2024 · 11 comments

Comments

@SimonHeybrock
Copy link
Member

Currently many "types" are in esssans.types, and we use from esssans.types import *. Apart form wanting to avoid the import *, types is also meaningless to scientists. Therefore:

  • Move important types to the package root (import in __init__.py), avoid import *.
  • More special or internal types should move to meaningful submodules.
@SimonHeybrock SimonHeybrock added this to the Essentials milestone Feb 20, 2024
@nvaytet nvaytet self-assigned this Mar 5, 2024
@nvaytet
Copy link
Member

nvaytet commented Mar 5, 2024

Avoiding from esssans.types import * would mean that setting parameters

# File names
params[Filename[SampleRun]] = '60339-2022-02-28_2215.nxs'
params[Filename[BackgroundRun]] = '60393-2022-02-28_2215.nxs'
params[Filename[TransmissionRun[SampleRun]]] = '60394-2022-02-28_2215.nxs'
params[Filename[TransmissionRun[BackgroundRun]]] = '60392-2022-02-28_2215.nxs'
params[Filename[EmptyBeamRun]] = '60392-2022-02-28_2215.nxs'

would become more cumbersome?

from esssans.io import types as iotypes
from esssans.normalization import TransmissionRun
# File names
params[iotypes.Filename[SampleRun]] = '60339-2022-02-28_2215.nxs'
params[iotypes.Filename[BackgroundRun]] = '60393-2022-02-28_2215.nxs'
params[iotypes.Filename[TransmissionRun[SampleRun]]] = '60394-2022-02-28_2215.nxs'

In addition, I think if we scatter all the types in different places, inspecting intermediate results may become more difficult.
For example, as it is now on main, if I make the SANS2d pipeline, and I want to inspect the ElasticCoordTransformGraph using pipeline.compute(ElasticCoordTransformGraph), it fails because ElasticCoordTransformGraph is not defined. I have to know, as a user, that I need to do

from esssans.conversions import ElasticCoordTransformGraph
res = pipeline.compute(ElasticCoordTransformGraph)

Wouldn't we be making it even worse if one has to remember where to find the types in all the different submodules?

@nvaytet nvaytet moved this from Triage to In progress in Development Board Mar 5, 2024
@SimonHeybrock
Copy link
Member Author

Why would you use Filename from iotypes? Filename should be imported in the top-level __init__.py, since it is commonly used by users.

@SimonHeybrock
Copy link
Member Author

To reiterate form previous discussions:

  • Define where it makes sense (use your judgement), might be in local file defining providers.
  • types should not be exposed to users.
  • "Relevant" types get imported in top-level __init__.py.
  • Details have to be imported from their submodules.

@nvaytet
Copy link
Member

nvaytet commented Mar 6, 2024

Filename should be imported in the top-level init.py, since it is commonly used by users.

I've tried a couple of things, but I can't seem to get around having to have a long

from esssans import (
    Filename,
    SampleRun,
    BackgroundRun,
    TransmissionRun,
    EmptyBeamRun,
    WavelengthBins,
    WavelengthBands,
    CorrectForGravity,
    UncertaintyBroadcastMode,
    ReturnEvents,
    QBins,
    PixelMaskFilename,
    BackgroundSubtractedIofQ,
)

at the start of the notebook. Was that the intention?

Another thing: without the import *, if I want to inspect an intermediate result that I look up in the task graph, I now have to import that type before I can look at the data

from esssans.types import MaskedData
masked = pipeline.compute(MaskedData[SampleRun])

which degrades usability I think.

I know we don't like the import *, but I feel the alternative should not lower usability this much?

@SimonHeybrock
Copy link
Member Author

It is your choice to import *, but I don't feel we should adopt that practice in the docs?

@nvaytet
Copy link
Member

nvaytet commented Mar 6, 2024

It is your choice to import *

Sure, but that becomes very difficult if the types are scattered over many files/submodules internally.

So unless we find a better alternative, I might suggest to have, for every submodule, a single place where we put all the types (and we don't have to call it types if you think the name doesn't make sense to scientists).

This would allow you to do:

from esssans.types import *
from esssans.loki.types import *

and you would have all you need if you are working with Loki. We would then not have any types in the other files in the loki submodule.

I agree that we should leave it out of the docs 👍 , but I think you should still be able to use it because it is very convenient?

@SimonHeybrock
Copy link
Member Author

but I think you should still be able to use it because it is very convenient?

Not sure what you are saying, we cannot possible prevent people from doing from ess.sans import *, right?

@SimonHeybrock
Copy link
Member Author

It is your choice to import *

Sure, but that becomes very difficult if the types are scattered over many files/submodules internally.

So unless we find a better alternative, I might suggest to have, for every submodule, a single place where we put all the types (and we don't have to call it types if you think the name doesn't make sense to scientists).

This would allow you to do:

from esssans.types import *
from esssans.loki.types import *

I really don't see why we need types. Can't almost all relevant bits be in the relevant module?

from esssans import *
from esssans.loki import *

@nvaytet
Copy link
Member

nvaytet commented Mar 6, 2024

Can't almost all relevant bits be in the relevant module?

In theory, yes,
But it's inconvenient if I want to inspect an interediate result which is not imported at the submodule top level, and the type is burried in e.g. esssans.loki.masking.
A user would either have to grep the source (most won't know where to find it in their conda/pip environment), or search the docs API to try and find the correct submodule?

Inspecting intermediate results will be very common in hot commissioning.

@nvaytet
Copy link
Member

nvaytet commented Mar 6, 2024

Regarding not being able to easily find the full path to a type when looking at a task graph:

I also tried adding 'fake' url to the types nodes, where you could right-click on the svg and do "copy link address", but instead of a http url it would just be the full type path that you could then paste back into your notebook and use compute on.
While being a simple fix in sciline, chrome seems to just plainly block those urls. Firefox allows them but pre-pends the local folder path which means you get file:///home/nvaytet/code/sans/jupyter/esssans.types.Filename[esssans.types.SampleRun] instead of esssans.types.Filename[esssans.types.SampleRun] :-(

@nvaytet
Copy link
Member

nvaytet commented Mar 6, 2024

The conclusion was that we will wait with this until we have more experience with more workflows. Moving to blocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Blocked
Development

No branches or pull requests

2 participants