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

Add documentation on putting text onto images #2517

Merged
merged 12 commits into from
Oct 17, 2023
Merged

Conversation

simonrp84
Copy link
Member

As discussed on slack, this PR adds a brief example to the documentation to describe how text can be added to an image when using save_dataset.

@simonrp84 simonrp84 marked this pull request as ready for review June 21, 2023 20:46
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #2517 (9286e4d) into main (93bae5e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2517   +/-   ##
=======================================
  Coverage   94.94%   94.94%           
=======================================
  Files         354      354           
  Lines       51474    51474           
=======================================
  Hits        48873    48873           
  Misses       2601     2601           
Flag Coverage Δ
behaviourtests 4.27% <ø> (ø)
unittests 95.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
satpy/dataset/dataid.py 95.39% <ø> (ø)
satpy/readers/modis_l2.py 89.90% <ø> (ø)
satpy/readers/seviri_base.py 100.00% <ø> (ø)
satpy/scene.py 93.59% <ø> (ø)

@coveralls
Copy link

coveralls commented Jun 21, 2023

Pull Request Test Coverage Report for Build 5343898810

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.421%

Totals Coverage Status
Change from base Build 5338770761: 0.0%
Covered Lines: 47201
Relevant Lines: 49466

💛 - Coveralls

doc/source/writers.rst Outdated Show resolved Hide resolved
doc/source/writers.rst Outdated Show resolved Hide resolved
doc/source/writers.rst Outdated Show resolved Hide resolved
@gerritholl
Copy link
Collaborator

Should we change the name writers? The section is not only about writers, but about writing/saving images/products.

@djhoese
Copy link
Member

djhoese commented Jun 26, 2023

Should we change the name writers? The section is not only about writers, but about writing/saving images/products.

This is difficult to answer. The confusion of the documentation page is mostly due to the poorly designed interfaces. That is, that you add overlays at save/write time rather than as a separate step. I don't think it is completely wrong where it is, but yeah maybe a name change to Writing would help?

@mraspaud
Copy link
Member

Should we change the name writers? The section is not only about writers, but about writing/saving images/products.

This is difficult to answer. The confusion of the documentation page is mostly due to the poorly designed interfaces. That is, that you add overlays at save/write time rather than as a separate step. I don't think it is completely wrong where it is, but yeah maybe a name change to Writing would help?

I think Writing is a good name. @gerritholl any comments?

@mraspaud
Copy link
Member

@simonrp84 not much left to finalise this one, want to give it a shot?

@mraspaud mraspaud added this to the v0.44.0 milestone Oct 11, 2023
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Looks pretty good. You'll need to update the references to the reading and writing files in the index.rst file that points to them.

I made an inline comment for some changes. Additionally, do we want to go down the road of also documenting how to add coastlines? I suppose that would be easier when pycoast gets rewritten to automatically download/cache coastline shapefiles (a long term dream of mine), but that could be a while.


.. code-block:: python

>>> decodict = {'decorate': [{'text': {'txt': f' {my_text}',
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a simple string to not confuse people? Just "txt": "my text"?

And...based on the conversation at the meeting yesterday and @mraspaud's PR on the ruff linting tool, can we use double quotes for all of these strings please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@simonrp84
Copy link
Member Author

In general I like the idea of adding documentation on coastlines (I always end up having to search github issues for it). But I'm not sure we should add it in this PR, as I don't really have any time to work on it and we'd end up delaying the PR.

Will address your inline comment this evening.

@mraspaud
Copy link
Member

I see also that the rtd build is failing, but I can't see what cause the error...

@djhoese
Copy link
Member

djhoese commented Oct 11, 2023

@mraspaud In my comment above:

You'll need to update the references to the reading and writing files in the index.rst file that points to them.

@mraspaud
Copy link
Member

@simonrp84 the rtd build is still failing...

doc/source/reading.rst Outdated Show resolved Hide resolved
@djhoese
Copy link
Member

djhoese commented Oct 12, 2023

The remaining RTD errors are about docstrings referring to the old readers/writers.rst documents. Those fixes are left as an exercise for the reader.

@simonrp84
Copy link
Member Author

Ok, RTD seems to work now but was getting a failing test - I think due to numpy 2.0. Have merged main and hopefully that will fix it...

@mraspaud
Copy link
Member

@simonrp84 great job! the unstable build is still expected to fail so I'm merging this!

@mraspaud mraspaud merged commit 6307a27 into pytroll:main Oct 17, 2023
19 of 20 checks passed
@simonrp84
Copy link
Member Author

Thanks!

@simonrp84 simonrp84 deleted the doco_upd branch October 17, 2023 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants