Skip to content

Improve Observation API and documentation#84

Merged
ziotom78 merged 13 commits intomasterfrom
obs_doc
Jan 9, 2021
Merged

Improve Observation API and documentation#84
ziotom78 merged 13 commits intomasterfrom
obs_doc

Conversation

@dpole
Copy link
Copy Markdown
Member

@dpole dpole commented Jan 7, 2021

This PR clarifies the documentation of the Observation class. It also changes its API for better clarity and coherency. As a part of this, al the pointing-related methods are turned into functions of the scanning module

dpole added 9 commits January 5, 2021 18:40
* New (consistent) convention: unless global is specified, all the
  attributes refer to local quantities
* Pointing related functionalities moved to the scanning module
@ziotom78
Copy link
Copy Markdown
Member

ziotom78 commented Jan 7, 2021

Thanks a lot, @dpole.

As a part of this, al the pointing-related methods are turned into functions of the scanning module

Regarding this, I believe it would be better for the moment to keep the class methods as tiny wrappers to the function, and to include a "deprecation" warning. A number of people are already using the scanning generator, and having warning messages would help them in porting the code. An example of a message could be the following:

DeprecationWarning: instead of writing "cur_obs.get_pointings(...)", please write "litebird_sim.get_pointings(cur_obs, ...)"

@dpole dpole changed the title Obs doc Improve Observation API and documentation Jan 7, 2021
Comment thread litebird_sim/observations.py Outdated
from .distribute import distribute_evenly
from .scanning import Spin2EclipticQuaternions, all_compute_pointing_and_polangle

# NOTE: remove these imports after the deprecated methods have been removed
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.

Wait, why should these imports be removed? They are referring to the functions, not to the methods of the Observation class, right?

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.

Yes, but once the deprecated methods are removed from Observation, they are not necessary anymore in this class. Right?

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.

Ok, but the comment is misleading, as it was placed above the exported functions, not the methods.

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.

Ah ok, now I see your point, I'll clarify.

Comment thread litebird_sim/observations.py Outdated
"""
logging.warn(
"Observation.get_quaternion_buffer_shape is deprecated and will be "
"removed soon, user scanning.get_quaternion_buffer_shape instead")
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.

"user" -> "use" (the same typo happens below a few times)

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.

Fixing, thanks

@dpole
Copy link
Copy Markdown
Member Author

dpole commented Jan 7, 2021

I think this PR is ready to be merged, please feel free to comment, especially about the API. Note that the updated doc is already available here

@ziotom78
Copy link
Copy Markdown
Member

ziotom78 commented Jan 8, 2021

Great, thanks! Feel free to merge it, but first please add a note in file CHANGELOG.md about the deprecated methods of the Observation class.

@dpole
Copy link
Copy Markdown
Member Author

dpole commented Jan 8, 2021

Ok, I will. I also noticed that there is problem with readthedocs. Building the documentation often (and unpredictably) results in the following error

Building wheels for collected packages: litebird-sim, backports-datetime-fromisoformat, ducc0, jplephem, katex, pyrsistent, pyyaml, oset, wrapt, pandocfilters
  Building wheel for litebird-sim (PEP 517): started
  Building wheel for litebird-sim (PEP 517): finished with status 'done'
  Created wheel for litebird-sim: filename=litebird_sim-0.2.0-py3-none-any.whl size=355170 sha256=37700a1f1e612cf28d7df8e895d9693f1269c283b6945d7bcc872972083d02b8
  Stored in directory: /tmp/pip-ephem-wheel-cache-51uh0rz4/wheels/84/a9/2e/c098a8ebcc99e4ae306f9b6faefb437a396a977f64df0b70d2
  Building wheel for backports-datetime-fromisoformat (setup.py): started
  Building wheel for backports-datetime-fromisoformat (setup.py): finished with status 'done'
  Created wheel for backports-datetime-fromisoformat: filename=backports_datetime_fromisoformat-1.0.0-cp37-cp37m-linux_x86_64.whl size=36783 sha256=5a57fab230b8c11b2c425b713e4101a3f729117302c615fcf098bd331cb04505
  Stored in directory: /tmp/pip-ephem-wheel-cache-51uh0rz4/wheels/b8/e4/4a/14e3dc574ae5ca155b442a3f899078f51a08b00d80ea821ff6
  Building wheel for ducc0 (PEP 517): started


Command killed due to excessive memory consumption.

I would merge anyway because it is not a problem with the PR itself.

@ziotom78 ziotom78 merged commit 7ad2983 into master Jan 9, 2021
@ziotom78 ziotom78 deleted the obs_doc branch January 9, 2021 04:37
@ziotom78
Copy link
Copy Markdown
Member

ziotom78 commented Jan 9, 2021

The problem with Readthedocs is likely due to what was spotted in #89.

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.

2 participants