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

Define indexing functionality #22

Merged
merged 4 commits into from
May 14, 2021
Merged

Define indexing functionality #22

merged 4 commits into from
May 14, 2021

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented May 13, 2021

This is to make it easy to work with the output of checkpointing.
No mode adhock walkdir or use of Glob.jl.
Increased robustness against the addition of new tags, or reordering of tags.

TODO:

  • bikeshed over names (this will want to export all of its field access helpers, it would be annoying if they clashed too often with other exports)
  • document field access helpers
  • tests

@aisopous
Copy link
Contributor

aisopous commented May 13, 2021

My earlier use-case for a similar type was to make the parsing of multiple different kind of things quick from a single checkpoint. In particular, I wanted to do

checkpoint = read_checkpoint(path)
f = Forecaster(checkpoint)
n = Nodes(checkpoint)
dates = checkpointed_dates(checkpoint)
y = forecast(f, n, dates)

with only a single call to S3.

src/indexing.jl Outdated Show resolved Hide resolved
src/indexing.jl Outdated Show resolved Hide resolved
src/indexing.jl Outdated Show resolved Hide resolved
Copy link
Contributor

@mzgubic mzgubic left a comment

Choose a reason for hiding this comment

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

I really like this addition!

src/indexing.jl Outdated Show resolved Hide resolved
src/indexing.jl Outdated Show resolved Hide resolved
src/indexing.jl Outdated Show resolved Hide resolved
src/indexing.jl Show resolved Hide resolved
path::P
name::AbstractString
groups::NTuple{<:Any, AbstractString}
tags::NTuple{<:Any, Pair{Symbol, <:AbstractString}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this is nicer, do you mind changing this line to be consistent across the package?

@contextvar CONTEXT_TAGS::Tuple{Vararg{Pair{Symbol, Any}}} = Tuple{}()

Copy link
Member Author

Choose a reason for hiding this comment

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

is it? those are the basically same

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah sure - yours reads more nicely

Copy link
Member Author

Choose a reason for hiding this comment

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

I think mine might break, because i contrain them all to be the same abstract sttring subtype, which I know will work in this case, because they are all SubStrings, but might not in others.

src/indexing.jl Outdated
)
elseif length(inds) == 0
if hasfield(CheckpointOutput, name)
# as long as we don't have a tag with one of the field names we pass it through
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this. Sure, the fields are not public API, but changing the behaviour given which tags are used seems evil. How about we error here and tell the user to use the methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could.
Main reason I don't want to is so that tables work.
But we can make tables fail in that case also.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I leave it up to you to decide. LGTM overall

Copy link
Member Author

Choose a reason for hiding this comment

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

I have made that change

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #22 (a26ec3b) into main (0485d7c) will decrease coverage by 0.20%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
- Coverage   95.09%   94.89%   -0.21%     
==========================================
  Files           4        5       +1     
  Lines         102      137      +35     
==========================================
+ Hits           97      130      +33     
- Misses          5        7       +2     
Impacted Files Coverage Δ
src/Checkpoints.jl 91.66% <ø> (+0.23%) ⬆️
src/indexing.jl 94.11% <94.11%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0485d7c...a26ec3b. Read the comment docs.

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.

None yet

4 participants