Remove mentions of attr from docs #3263
Conversation
2e4b890 to
720542d
Compare
0cb8171 to
f55658b
Compare
nvaytet
left a comment
There was a problem hiding this comment.
Looks good overall. Some small comments. I have to say I did not go through the scipp source to see if any other mentions of attributes were missed, I only checked that what is in this PR looks ok.
| "ds.coords['x'] # the coords *dict* is copied, but the 'x' coordinate references same buffer" | ||
| "# the coords *dict* is copied, \n", | ||
| "# but the 'x' coordinate references same buffer\n", | ||
| "ds.coords['x'] " |
There was a problem hiding this comment.
| "ds.coords['x'] " | |
| "ds.coords['x']" |
| ) | ||
| new['flares'].coords['max_energy'] = new['flares'].attrs.pop('max_energy') | ||
| new['flares'].coords['min_energy'] = new['flares'].attrs.pop('min_energy') | ||
| new.save_hdf5("docs/tutorials/data/rhessi_flares.h5") |
There was a problem hiding this comment.
| new.save_hdf5("docs/tutorials/data/rhessi_flares.h5") | |
| new.save_hdf5("rhessi_flares.h5") |
?
Or have a common path prepended to both input and output files?
| "- `peak_time`: Date and time of the highest x-ray flux.\n", | ||
| "- `x`, `y`: Position in the image seen by RHESSI.\n", | ||
| "- `min_energy`, `max_energy`: Energy band that a flare was observed in. Bands do not overlap." | ||
| ] |
There was a problem hiding this comment.
Missing non-solar? I can see we are still using it below.
There was a problem hiding this comment.
Ah, is it because it's not longer a coordinate? Do we need to describe its meaning anyway if we are using it?
There was a problem hiding this comment.
Ah, is it because it's not longer a coordinate? Do we need to describe its meaning anyway if we are using it?
There was a problem hiding this comment.
Ah, is it because it's not longer a coordinate?
Yes. I meant to explain it separately but forgot. Did it now.
docs/tutorials/solar_flares.ipynb
Outdated
| "# Move min_energy from attributes to coordinates\n", | ||
| "flares.coords['min_energy'] = flares.attrs.pop('min_energy')\n", | ||
| "flare_data = sc.io.load_hdf5(filename)\n", | ||
| "flare_data" |
There was a problem hiding this comment.
I think it's slightly confusing for new-comers between flare_data and flares.data. Can we make the names more different?
There was a problem hiding this comment.
I always struggle to come up with good names in this situation. It's similar to loading nexus files:
<needs-a-name> = load()
detector = <needs-a-name>['instrument']['detector']Do you have a good idea what to use for <needs-a-name>?
There was a problem hiding this comment.
Do you have a good idea what to use for
<needs-a-name>?
No.
I see in this PR you changed to flare_datagroup, which is fine. I guess you could also have used something like flare_database, if you wanted to stay away from the Scipp types, but either is fine.
docs/user-guide/computation.ipynb
Outdated
| "| mask | combine with `or` |\n", | ||
| "| attr | typically ignored or dropped |\n", | ||
| "\n", | ||
| "In the special case of in-place operations such as `+=` or `*=` Scipp preserves existing attributes and ignores attributes of the right-hand-side.\n", |
There was a problem hiding this comment.
Remove this sentence as well? It's talking about attributes.
| "When slicing a data array the following additional rule applies:\n", | ||
| "\n", | ||
| "- Meta data (coords, masks, attrs) that *do not depend on the slice dimension* are marked as *readonly*\n", | ||
| "- Meta data (coords, masks) that *do not depend on the slice dimension* are marked as *readonly*\n", |
There was a problem hiding this comment.
What should be the consistent spelling? "Meta data" or "Metadata"?
There was a problem hiding this comment.
According to my grammar checker, Wikipedia, and Merriam-Webster it's 'metadata'.
It's inconsistent in our docs.
nvaytet
left a comment
There was a problem hiding this comment.
Looks good but there are merge conflicts....
1141f2b to
39d7645
Compare
Part of #3166.
I did not yet touch the data creation scripts
prepare_data_rhessi.pyandprepare-filtering-data.pyand I haven't looked in detail at the latter yet.Also, I did not touch ADRs after talking to Neil about it. We agreed that ADRs should not change after they were accepted.
EDIT
Migrated the event filtering example.
EDIT
Migrated the solar flares example.
Unfortunately, NASA updated the flare list and it is now filtered such that the tutorial doesn't make much sense. So I stuck with the old data. But since I don't have a copy of the old flare list, I modified our old hdf5 file.