plenoptic-org / plenoptic

Visualize/test models for visual representation by synthesizing images.
https://plenoptic.readthedocs.io/en/latest/
MIT License
57 stars 9 forks source link

Improve API #246

Open billbrod opened 7 months ago

billbrod commented 7 months ago

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 commented 6 months ago

This is related to issues #128, #97

billbrod commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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