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

Refactor to have references in separate sub-directory; closes #49 #77

Merged
merged 12 commits into from
Aug 2, 2024

Conversation

dr-rodriguez
Copy link
Collaborator

@dr-rodriguez dr-rodriguez commented Jul 23, 2024

This changes the database saving/loading logic to add an optional reference_database variable (defaults to "reference"). When set, any table set as reference will get stored in that sub-directory. This is backwards compatible in the sense that when loading a database, it will also check the parent directory if it doesn't file reference tables in the sub-directory.

Closes #49

@dr-rodriguez dr-rodriguez marked this pull request as ready for review July 23, 2024 20:59
@dr-rodriguez dr-rodriguez self-assigned this Jul 23, 2024
@dr-rodriguez dr-rodriguez requested a review from kelle July 23, 2024 20:59
@dr-rodriguez dr-rodriguez changed the title First pass at having references in separate directory Refactor to have references in separate sub-directory Jul 23, 2024
@dr-rodriguez dr-rodriguez changed the title Refactor to have references in separate sub-directory Refactor to have references in separate sub-directory; closes #49 Jul 23, 2024
@kelle
Copy link
Collaborator

kelle commented Jul 25, 2024

This looks great! One question, do we really want reference/ to be a sub-directory of data/? This structure doesn't necessarily help with our navigation challenge.

SIMPLE-db/
├─ data/
│  ├─ 2mass source.json
│  ├─ asource.json
│  ├─ source1.json
│  ├─ reference_tables/
│  ├─ source2.json

Maybe we should we also have a sources sub-directory?

SIMPLE-db/
├─ data/
│  ├─ Sources/
│  │  ├─ 2mass source.json
│  │  ├─ source2.json
│  │  ├─ asource.json
│  │  ├─ source1.json
│  ├─ Reference_Tables/
│  │  ├─ Publications.json
│  │  ├─ Telescopes.json

Another option is to make the data/ and the reference/ directories both at the top level.

SIMPLE-db/
├─ data/
│  ├─ 2mass source.json
│  ├─ asource.json
│  ├─ source1.json
│  ├─ source2.json
├─ reference_tables/
│  ├─ Publications.json
│  ├─ Telescopes.json

@dr-rodriguez
Copy link
Collaborator Author

dr-rodriguez commented Jul 28, 2024

I do think they should be under data/ since both source data and references are data; having a separate top-level directory could lead to loosing part of the data.
Having a second sub-directory for the sources is something I thought about, but didn't try out. Seeing how relatively simple building this was I can probably do the same for the non-reference tables.

I can explore that over the next week. For ease of comparison I'll probably create a new branch off of this one, but we ideally want to merge both together.

So with that in place, we would have sources/ and references/ both under data/ which I thinkn is probably the cleanest setup. (We can iterate if those should be the best sub-directory names)

@dr-rodriguez dr-rodriguez requested a review from arjunsavel August 2, 2024 17:04
@dr-rodriguez
Copy link
Collaborator Author

This is now ready to go. It stores references under data/reference and sources under data/source. Tests are passing and I was able to create a PR for SIMPLE using this refactor at SIMPLE-AstroDB/SIMPLE-db#541

Once this is merged, I'll make a major astrodbkit2 release and re-run the SIMPLE tests.

Copy link
Collaborator

@kelle kelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added some suggested wording to the documentation. I've seen lots of folks struggle with paths and directory structure so I tried to make the structure more explicit throughout.

astrodbkit2/astrodb.py Outdated Show resolved Hide resolved
astrodbkit2/astrodb.py Outdated Show resolved Hide resolved
astrodbkit2/astrodb.py Outdated Show resolved Hide resolved
astrodbkit2/astrodb.py Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@dr-rodriguez dr-rodriguez merged commit 4043a64 into astrodbtoolkit:main Aug 2, 2024
4 checks passed
@dr-rodriguez dr-rodriguez deleted the reference-directory branch August 2, 2024 18:13
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.

Move reference tables out of data/ directory
2 participants