Skip to content

Add support for multi-file concatenation for NCCatalog#385

Merged
ph-kev merged 2 commits into
mainfrom
kp/catalog
Mar 30, 2026
Merged

Add support for multi-file concatenation for NCCatalog#385
ph-kev merged 2 commits into
mainfrom
kp/catalog

Conversation

@ph-kev

@ph-kev ph-kev commented Mar 11, 2026

Copy link
Copy Markdown
Member

closes #322 - This PR adds support for multi-file concatenation for NCCatalog by reusing the same functionality in read_var.

@ph-kev ph-kev requested a review from imreddyTeja March 11, 2026 23:52
@codecov

codecov Bot commented Mar 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.45%. Comparing base (c2ddb28) to head (e8c229d).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #385   +/-   ##
=======================================
  Coverage   99.45%   99.45%           
=======================================
  Files          18       18           
  Lines        2005     2028   +23     
=======================================
+ Hits         1994     2017   +23     
  Misses         11       11           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@imreddyTeja

Copy link
Copy Markdown
Member

What does mfds stand for? Multi-file data struct?

@ph-kev

ph-kev commented Mar 17, 2026

Copy link
Copy Markdown
Member Author

What does mfds stand for? Multi-file data struct?

Multi file dataset

@imreddyTeja imreddyTeja left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would this be a breaking change because NCCatalog is an exported struct?

Comment thread src/Catalog.jl Outdated
if isempty(maybe_short_name_pairs)
short_name = nothing
else
maybe_short_name_pair = first(short_names)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know this is not a new addition, but I'm a bit confused on what this is doing

@ph-kev ph-kev Mar 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I need to get a short name to figure out what the time dimension is. There is probably a better way of doing this now that I think about it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I used the short name since I needed to check if the start_date attribute is consistent between all the datasets. I think a better way of doing this is to scan all the variable names and check that all of them has a start date or don't has a start date.

@ph-kev

ph-kev commented Mar 17, 2026

Copy link
Copy Markdown
Member Author

Would this be a breaking change because NCCatalog is an exported struct?

I don't think this will be a breaking change? There are changes to the internals of the struct, but none of our code make use of the internals and they shouldn't anyway.

@ph-kev ph-kev force-pushed the kp/catalog branch 2 times, most recently from 09b02f5 to c29fade Compare March 19, 2026 17:52
@ph-kev ph-kev requested a review from imreddyTeja March 19, 2026 17:52
@ph-kev ph-kev force-pushed the kp/catalog branch 2 times, most recently from 4497767 to 970e191 Compare March 19, 2026 17:57

@imreddyTeja imreddyTeja left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One nit about some wording

Comment thread src/Var.jl
@ph-kev ph-kev merged commit adde10b into main Mar 30, 2026
11 checks passed
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.

Add functionality for NCCatalog to stitch together multiple datasets together

2 participants