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

Initial rearranging of files #57

Merged
merged 4 commits into from
Apr 27, 2020
Merged

Initial rearranging of files #57

merged 4 commits into from
Apr 27, 2020

Conversation

tskisner
Copy link
Member

Not yet ready for review or merge, still need to fix all the imports. Working on rearranging existing files and only deleting things which were obviously abandoned. The actual code modifications will be in a later PR.

@tskisner
Copy link
Member Author

Ok, @mhasself , can you check this out? I split the imports of sotoddb between core.metadata and io.metadata. I also moved context.py into I/O, since it was using databases and yaml files.

No attempt in this PR to reconcile the metadata / Hardware split.

@tskisner tskisner requested a review from mhasself April 16, 2020 22:19
@mhasself
Copy link
Member

Thanks -- taking a look. I'm inclined to put Context and basically all sotoddb stuff into core, not io. The functionality in those modules is intended to support abstraction, even if in some cases there is self-i/o built in (such as the built-in methods for reading/writing each database type). I tend to see the io module as the place to implement various loaders -- the ones that use MPI and the ones that don't, for example. But the abstraction provided by Context and the supporting databases is more universal. I think simple.py is the only bit that clearly belongs in io. Does that make sense?

@mhasself
Copy link
Member

Not sure how deep we want to make this hierarchy, but how do you feel about:

   from sotodlib import core
   core.Context(...)        # This should be very easy to access
   core.context.ObsDb(...)  # Ok to bury the supporting DBs in .something,
                            # as long as no separate import is required.

While we're at it, let's rename all the SomethingDB to SomethingDb... e.g. I think I've been convinced that camel case for acronyms is the smart thing to do when one speaks almost entirely in acronyms, TodDbLol.

I guess this would mean

  • creating core/context/
  • putting context.py in there under a different name
  • importing Context and *Db (in the form from sotoddb import ObsDB as ObsDb and similar) in core/context/__init__.py
  • importing context and Context in core/__init__.py.

@tskisner
Copy link
Member Author

tskisner commented Apr 17, 2020

Ok, now I'm confused about the policy... I think originally we had:

  • core is for containers and other classes used throughout the package, but not reading / writing data

  • io is for reading and writing

An AxisManager class (for example) can be constructed and used without touching disk. A Context is essentially an interface to files on disk. You can't even construct a Context without reading files.

Made-up example: imagine we had a class which was used throughout sotodlib that represented detector beams. The constructor of this class took the path to a potentially large file on disk and loaded the file during construction. Does this class belong in core or io?

If you answered "core", then I guess that means that "io" is really only for code doing I/O of time ordered data. Is that your intention?

Side note: I just realized that the top-level g3_core.py should go in "core" as well, since it is a container. Remember, this PR is just trying to get existing files into the right place, and then we can work on the code itself.

@tskisner
Copy link
Member Author

I'll rebase on master and resolve conflicts once we sort this out. It sounds like you want to make all of the existing sotoddb code live with the Context class, which I think makes sense since it seems like that is the only place where it is used, correct? I agree in that case that a context subpackage with the Context class and all the DB stuff makes sense.

@mhasself
Copy link
Member

Right... I think some objects, such as framework engine classes like the context databases, have simple formats on disk that can be included in the class even if the class is considered "core"... (Actually I think Context should be constructable without a filename, it's a bug that it isn't.)

Yes, I suppose I am thinking of io primarily as the place where our diverse set of TOD loading routines will live. Since our format will inevitably evolve, and since we're dealing with multiple large datasets (if you include TOD sims), we can't get away with just one function that does everything. We will need routines that can load each iteration of our data format.

I think some metadata I/O stuff will live in io too (such as the HDF5 business implemented in sotoddb.simple).

the proposed layout in #53.  Only delete files that were
obviously empty or never used.  No changes or cleanup to
the code here, just moving things around and changing
imports so that unit tests pass.
@tskisner
Copy link
Member Author

Ok, I just moved context.py into core. I know you wanted to further split that up, but in this PR I just want to move around existing files and ensure that the tests still run. Then we can take the next step of reorganization.

@tskisner
Copy link
Member Author

@mhasself , I think this is ready for review. Again, this PR is just moving existing files around. Then we can open separate PRs for changing those files.

This imports ResultSet and the metadata *Db directly to core.metadata,
renaming the *DB classes as *Db.  The more stuff-on-disk code in
sotodlib.simple is imported as io.metadata.simple.  This has been
tested on simons1 with the SO sim and ACT Uranus demo code.
Copy link
Member

@mhasself mhasself 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, thanks. I have added a commit that moves around (and renames) the sotoddb imports a little. Please take a look at that before merging!

If testing, might require the "pre-integration-tweaks" branch of sotoddb.

@tskisner
Copy link
Member Author

Ok, those import changes look fine to me, and let's get this merged so that we can move on to the bulk of the other issues.

@tskisner tskisner merged commit 725af16 into master Apr 27, 2020
@tskisner tskisner deleted the issue_56 branch April 27, 2020 17:59
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.

3 participants