Settles part of the discussion from #143 that we want to settle.#147
Conversation
Set this Python version to exclude false warnings from older Python versions.
This commit should enforce that if all three of bmaj, bmin and bpa are set as part of a ImgConf instance, they are propagated into "beam". Previously, this propagation was not enforced. "extract_metadata" will return metadata without beam information if not all three of bmaj, bmin and bpa are set as part of a ImgConf instance.
1) If (bmaj(degrees), bmin(degrees), bpa(degrees)) has been supplied as an argument this is used to compute (smaj(pixels), smin(pixels), theta(radians)). (Both of these 3-tuples are referred to as "beam", but they have different units) 2) If there is no "beam" as an argument, use (bmaj(degrees), bmin(degrees), bpa(degrees)) from config.toml (or command-line with cli.py). 3) Last option: extract (bmaj(degrees), bmin(degrees), bpa(degrees)) from FITS or CASA table header.
| print(("WARNING: Partial beam specification ignored; " | ||
| "one or more of (bmaj, bmin, bpa) are not " | ||
| "specified.")) |
There was a problem hiding this comment.
If you make it a warnings.warn instead of print it allows downstream users to filter on it
| print(("WARNING: Partial beam specification ignored; " | |
| "one or more of (bmaj, bmin, bpa) are not " | |
| "specified.")) | |
| warnings.warn(("Partial beam specification ignored; " | |
| "one or more of (bmaj, bmin, bpa) are not " | |
| "specified."), RuntimeWarning) |
There was a problem hiding this comment.
Right, much better.
Will include in upcoming commit.
There was a problem hiding this comment.
But perhaps a RunTimeWarning should not be raised here, since the beam can be filled in "further down the line".
There was a problem hiding this comment.
Do we expect the user to update the beam conf after creating the object? If so we could decide to define a property getter that raises a warning if some parts of the beam are None, but at least the way I use it now is that I define the pyse conf before calling pyse functions and do not modify that afterwards. Is there a usecase where this does happen?
There was a problem hiding this comment.
I was thinking of this use case:
some_im = sourcefinder_image_from_accessor(FitsImage('test/data/image_206-215-t0002.fits',
beam=(0.208, 0.13, 15.619)))
There was a problem hiding this comment.
Ah I see. In that case I think we can cover this by adding a similar warning here:
Line 91 in f3bad7b
Or you could remove all usages of the beam as a function argument and use the info in ImgConf instead. Then you could add a property to the ImgConf that looks someting like:
@dataclass(frozen=True)
class ImgConf(_Validate):
bmaj: float | None = None # Set beam: Major axis of
# restoring beam (degrees).
bmin: float | None = None # Set beam: Minor axis of
# restoring beam (degrees).
bpa: float | None = None # Set beam: Restoring beam position
@property
def beam(self):
beam = (self.bmaj, self.bmin, self.bpa)
if None in beam:
warnings.warn(("Partial beam specification ignored; "
"one or more of (bmaj, bmin, bpa) are not "
"specified."), RuntimeWarning)
return None
return beam
Which you can then reference like conf.image.beam. But with this approach passing the beam like you do in this example does not make sense anymore because it then needs to be part of the config object:
some_im = sourcefinder_image_from_accessor(FitsImage('test/data/image_206-215-t0002.fits',
beam=(0.208, 0.13, 15.619)))
So the way I see it you can either handle the beam in one place but break the current API or keep support for the keyword arguments in functions but perform check in multiple locations
There was a problem hiding this comment.
So the way I see it you can either handle the beam in one place but break the current API or keep support for the keyword arguments in functions but perform check in multiple locations
Thanks for elaborating. I am inclined to choose the latter, because beam is a special case in the sense that can it can be derived from an ImgConf object or extracted from a FITS image header / CASA table header.
For that reason, I think that maintaining the option of overriding beam is not bad.
There was a problem hiding this comment.
In that case I think we can cover this by adding a similar warning here:
Thanks. Will include in upcoming commit.
There was a problem hiding this comment.
Ah I see. In that case I think we can cover this by adding a similar warning here:
But then, since the image module is the "end point", shouldn't a RunTimeWarning there suffice, i.e. adding it anywhere else would be redundant?
There was a problem hiding this comment.
Yeah I think you are right. That would also solve it then :)
Added: 1) "_is_valid_beam_tuple" static method to "DataAccessor", should be a safeguard against ill-determined beams. 2) Some type annotation for linters.
Apply "_is_valid_beam_tuple" static private method from "DataAccessor" to three of its derived classes.
Replace "print" by "warnings.warn": "allows downstream users to filter on it", suggestion copied from @tmillenaar.
since we want to defer checking if "beam" is a valid 3-tuple of floats to "image" module, which is the "end point" for image processing. Also, "self.beam = None" is redundant, since that is default.
These three classes derived from "DataAccessor" should use the public method "is_valid_beam_tuple" instead of the private method, which has been removed.
It makes no sense to proceed with image processing if the beam has not been specified adequately, i.e. as a 3-tuple of floats (or doubles). The image module, being the "end point" for image processing - after the image has been loaded by other modules - seems the right place to abort image processing in that case.
"is_valid_beam_tuple" should be moved to prevent a circular import.
1) "is_valid_beam_tuple" now imported from utils 2) numpy --> np 3) etc --> etc. 4) "import warnings" has become redundant
"from scipy.sparse import bsr_matrix" must have been an error; sparse matrices are not being used.
2 participants
Quote from the #143 discussion:
That is not settled in this PR; after contemplation I wasn't sure I should choose the former or the latter.
What is solved here is the missing propagation of (bmaj, bmin, bpa) in a config.toml; this should settle the most important part of that discussion.