Closed matthewturk closed 9 months ago
wait the images are still shown as changed.
Also, for posterity: as discussed in #91, this shouldn't be merged before yt.load_sample is shipped (4.0)
a merge fixed it :)
Btw you may prefer to keep the full paths in every example. It's not required, but it produces identical results and may be considered best practice (depending on the example we want to give). Normally, the whole patch could just be about substituting yt.load(
with yt.load_sample(
, and if we don't remove the full path data, the reversed substitution will also work as long as the data is already installed (as was assumed in existing versions of the docs).
Hey, what do we think about this nowadays?
it's in better shape now that yt.load_sample
has been shipped for 4 releases over a year. However the function still requires two optional deps (pooch and pandas), and personally I don't like it that we force users to install pandas just for this (for the record it was my idea from 2 years ago), so I would like to refactor it so pandas isn't needed, but I digress.
We wrap ImportError
for both these dependencies so it limits the risk of confusion, so I wouldn't oppose this change now. Tell me if you want to add to it, otherwise I can just merge as is
@neutrinoceros looks like as of the last comment you're OK with this PR, that still true? @matthewturk how you feeling about it?
for what it's worth, +1 from me for being merged as-is at this point.
yes, go ahead. I never found the motivation to refactor pandas out of the function and at this point I doubt I ever will.
This updates the examples to use load_sample. Once #91 is included it'll be just those changes.