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

Improve API #246

Open
billbrod opened this issue Feb 14, 2024 · 6 comments
Open

Improve API #246

billbrod opened this issue Feb 14, 2024 · 6 comments

Comments

@billbrod
Copy link
Collaborator

From pyOpenSci review:

  • In general, accessing core classes and functions is verbose. I have been guilty of this too and I know this would in a sense be a breaking change, but I would strongly suggest importing core classes and functions at the top-level if possible. I want to be able to just type po.Metamer and po.remove_grad. It could be fine to leave them where they are but just import them
  • Remember file structure is an impelmentation detail and flat is better than nested
  • Reasonable people can disagree about this but I prefer to avoid namespaces with names like tools or util, because everything can go there, and as it stands now, you are just forcing users to type tools a lot. I would suggest moving modules in tools up a level and import the crucial functions directly into the plenopitc namespace from those modules. E.g. plenoptic.remove_grad. Or maybe plenoptic.grad.remove if you must have a module for it (seems weird that this function lives in validate).
  • it's also confusing and non-standard to internally import a sub-package at the root package level and change its name: e.g. synthesize -> synth and simulate -> simul. I kept thinking I missed an import statement with aliases, and I wasn't sure if there was something else going on with the code organization that I didn't know about. I would strongly suggest removing these internal aliases. If they are just too long, rename the module itself.
@billbrod
Copy link
Collaborator Author

This is related to issues #128, #97

@billbrod
Copy link
Collaborator Author

@NickleDave, I'd like your input on a potential restructure here:

plenoptic
   Metamer
   MADCompetition
   Geodesic
   Eigendistortion
   imshow
   animshow
   load_images
   remove_grad
   models
      all the classes found in models/ directory
   model_components
      everything found in canonical_computations/ directory
   metric
      everything found in metric/ directory
   conv
   display
   external
   ...all the other modules found in tools/ directory 

My concern with the above is I have 10 files found in tools/ (and I need to rename them, rearrange some of their functions). Do you think this would be too cluttered?

Separately, I have some helper functions for visualizing the results of synthesis optimization, such as plenoptic.simulate.metamer.plot_synthesis_status. These are all synthesis-specific (which is why they live in those specific files), but they're way too verbose to access. Do you have any suggestions as to how they should be accessed? They're not methods of the classes, because I didn't want to clutter those up -- I wanted the methods to all be synthesis/optimization-related, whereas these visualization ones all meant to be used after running optimization.

@NickleDave
Copy link

NickleDave commented Feb 22, 2024

Do you think this would be too cluttered?

This list is what would be imported into the top-level name space, inside plenoptic.__init__.py?
The list above doesn't seem too cluttered, if that covers the main functionality.

For the couple of times where you list "all the other modules", do you absolutely need to import those into the top-level name space? Maybe you could use lazy loading? (Not trying to add to your to-do list, just a thought)

edit: I don't think you definitely, totally, absolutely need to get rid of a tools sub-package, esp if that's a huge refactor -- sorry if that's what I implied. I would just import functions that live there at the top-level if they are frequently used. That might require a refactor if it causes circular imports tho

Separately, I have some helper functions for visualizing the results of synthesis optimization, such as plenoptic.simulate.metamer.plot_synthesis_status. Do you have any suggestions as to how they should be accessed?

Very subjective but I really like to have a top-level plot module and throw all the functions in there, so that it is concise. So, something like plenoptic.plot.metamer_synthesis_status or plenoptic.plot.metamer.synthesis_status. I know that only removes simulate -- does synthesis_status only ever apply to a metamer? If so maybe you could drop metamer too. The refactor would be annoying but it would make calling them more concise. Exactly because you want to interactively look at results e.g. after you run synthesis so you want to avoid being verbose there.

@billbrod
Copy link
Collaborator Author

billbrod commented Feb 22, 2024

I was planning on looking into lazy loading, @DanielaPamplona mentioned it. I need to look into exactly what it means -- from the user perspective, it's similar to importing them in to the top-level name space, right?

I think getting rid of tools makes sense, it is an odd grab-bag of unrelated functions. I need to think more deeply about how much work it requires, but at the minimum I will import the most-used functions at the top level.

For plot: I think that does make sense. I think I like having a flat plot module, there aren't too many plotting functions. Both metamer and mad competition have corresponding synthesis_status methods, and there are a couple other corresponding (but slightly different) plotting functions, so I think I would have a metamer_synthesis_status and mad_synthesis_status

@billbrod
Copy link
Collaborator Author

Related to this, we're currently ignoring Ruff error code F401 in the init files, but the right way to handle that is to set __all__, so do that as part of this refactoring.

@hmd101
Copy link
Contributor

hmd101 commented Oct 17, 2024

To remove wildcard (*) imports, the 2 recommended approaches are:

  1. Explicit Imports in the __init__.py File:
    - Manually specify the functions and classes to be imported.
    - Example: from .validate import remove_grad
    - This approach aligns with the flat hierarchy recommendation from the OpenSci review (see above)
  2. Nested Structure with Subpackages/Modules:
    - Organize the code into subpackages and import modules explicitly.
    - Example: plenoptic.tools.validate
    - Use from . import validate within the subpackage.
    - In files like validate.py, define a list called __all__ = ["remove_grad", ...], which specifies what can be imported from the file.
    - This can live in either the module file itself or the __init__.py, and it controls the public API of that module.

Note that the Ruff linterF403 flags wildcard imports * and that as part of the the PR #266 where the ruff linter is introduced, the this linting error is currently ignored.

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

No branches or pull requests

3 participants