Use "dataclasses" #60
Conversation
Using `@dataclass` vastly simplifies the implementation of `Detector`, and the new name `DetectorInfo` makes the connection with the IMO quantity clearer.
Apparently, Poetry 1.0 does not handle dependencies on specific versions of Python correctly: python-poetry/poetry#1413 While waiting for Poetry 1.1, this commit tries to fix the problem.
|
The classes
The reason why these are needed is because the fields in |
This is a **breaking** change: the `Instrument` class is renamed to `InstrumentInfo`, to match the names `DetectorInfo` and `FreqChannelInfo`. For consistency, the class has been moved from `instruments.py` to `detectors.py`, which already contained the definition for classes `DetectorInfo` and `FreqChannelInfo`. Moreover, the fields used by `InstrumentInfo` to hold angles can no longer be initialized with values in *degrees*: you must explicitly pass them as radians. This makes the interface to the class clearer: the parameters you pass to the constructor are the same you can access once the object has been constructed. The mock IMO used in tests has been updated to use the name `instrument_info` instead of `focal_plane`, to be consistent with the true IMO. A few new data files have been added to this IMO, to test the ability to produce data files. All the examples in the documentation have been updated accordingly.
This was missing from the previous commit. It implements a general-purpose way to produce a list of `DetectorInfo` objects out of a set of definitions in a dictionary, and it is meant to be used with TOML files. A new page about detectors, frequency channels, and instruments has been added to the documentation.
This might resolve a few problems when installing the "pillow" dependency under Mac OS X: python-pillow/Pillow#4242 (the title of the issue is misleading, as the problem does not affect only Windows user, as it's evident from the thread.)
|
The PR seems in good shape now. Please have a look at the documentation here: https://litebird-sim.readthedocs.io/en/dataclasses/detectors.html It will be merged mid next week, if no concerns arise. |
|
Hi, I've got two comments. Starting with the quick one, it seems to me that we should change name to Second, we touched the topic elsewhere, but I promised during the last call that I'll drop a line also here. I'm am a bit afraid of the proliferation of classes storing the information of some part of the instrument. As I said in #53 , I prefer bare data: it is less safe, less OO (which is actually a pro to me) but more transparent and minimal (in the sense that it requires the user to get familiar with fewer concepts). I am fine with creating specialized classes for central pieces of information (as in this PR), but, looking more into the future, I am wondering whether we have clear ideas about where to draw the line and how to handle information beyond that line. Otherwise we'll end up with one dataclass per IMO entity. |
|
Hi @dpole , a few comments:
While I agree that OO can make the codebase more difficult to understand (the old joke about the Finally, I too would like to avoid having too many classes, but my reason is against it is because of the "too many" part of the sentence, not because any OO construct must be avoided at all costs and in any case. I suggest we employ the following rule (abridged from here):
The three classes |
|
I see, thanks. I think we're talking about the same (or close enough) shade of gray from different angles. In particular, I agree that if we use dataclasses as C-like structures no complexity is added. This intention wasn't clear to me. A last question to clarify my practical concern about where to draw the line. Suppose that we add information about the HWP in the IMo with all sort of instrumental properties defined in it. How would you handle it? Is the idea to define a dedicated class? About my first comment, it wasn't about the plural but rather the fact that there is more than detectors in the module (namely channel and instrument). Finally...I am a bit disappointed because I didn't get the |
Yes, indeed: if the object is to be read from the IMO, it means that the set of metadata is "frozen" by a Specification Document, and thus it falls in the 1st category above. In this way, we can get the information about an HWP using the construct hwp_info = HwpInfo.from_imo(imo, "/ref/to/hwp")similarly to what we already do with Now I get the point about |
Maybe I'm overestimating the problem, but I imagine that the IMo will have a considerable number of different entities in the future. We are committing to writing a dataclass replicating the content of the imo specification for every entity. It seems to me a significant burden (maybe unnecessary) on developers. Beyond the effort issue, maybe it is somehow forbidden by some IMo specification, but I can imagine an entity with many child entities. Walking this hierarchy of entities and calling at each step the proper constructor can lead to quite a bit of complicated code. Having an "anonymous" data container into which any entity may be loaded using a generic function seems a more straightforward and "scalable" solution to me. |
I do not expect that many details in the IMO will need to be accessed by our framework. (E.g., mechanical structure of the many hardware parts, thermal models, optical models, RF models, circuit diagrams, component datasheets, etc.) We're going to implement dataclasses only for those modules we actually need for the simulation.
This framework is supposed only to load data files (not entities), and data files, like quantities, have no hierarchy in the current specification of the IMO. |
This PR adds the support for
dataclasses, a
feature of Python 3.7+ that is available on Python 3.6 too thanks to a
backward-compatible package.
This PR uses
@dataclassto define the following classes:DetectorInfo(previouslyDetector)FreqChannelInfoInstrumentInfoInstrument/InstrumentInfo(Proposal: a few breaking changes #65)