Add pylint #884
Add pylint #884
Conversation
…x and DesiTile to DesiData
|
Duplicated code is now removed. Overall I think the code is much simpler now. Before the merge is done, I need to update the data model with those changes, but this does not affect the code and it can be reviewed |
| from picca.delta_extraction.astronomical_objects.forest import Forest | ||
| from picca.delta_extraction.data_catalogues.desi_data import DesiData, defaults, accepted_options | ||
| from picca.delta_extraction.data_catalogues.desi_data import DesiData | ||
| from picca.delta_extraction.data_catalogues.desi_data import (# pylint: disable=unused-import |
There was a problem hiding this comment.
the split is just for line length? And the pragma is for pylint not to complain there because the import is multi-line?
There was a problem hiding this comment.
The split is because one is used in the module and the others are simply loaded (they are necessary imports but are not used inside the module). The pragma is for pylint to not complain about the fact the two imported variables are not used inside the module
There was a problem hiding this comment.
that's because they are only used for child classes? Or why would we elsewise import them here if they aren't used?
There was a problem hiding this comment.
They are (or could be) used by child classes. In any case, they are also used by Config in order to enforce correct parameters in the config file and give early errors
| weights = self.get_continuum_weights(forest, mean_expected_flux) | ||
| # this is needed as the weights from get_continuum_weights are | ||
| # divided by the continuum model squared, in this case mean_expected_flux | ||
| # TODO: check that we indeed need this or if the weights without it |
There was a problem hiding this comment.
Ok, so this does give the same result as before, right? It might be reasonable to have some kind of Fbar weights as an alternative, I guess... But this should be optional, not replacing the current setup
There was a problem hiding this comment.
Yes, this is to reproduce the same results as before, but it'd be nice to always use the same weights and not slightly different versions here and there
There was a problem hiding this comment.
While that would be nice, depending on the type of downstream analysis that can give unexpected effects. For Pk1d for instance, using the standard weights would be relatively simple to do when estimating the power spectrum from real space data, but when doing straight-forward FFTs each pixel of a spectrum will have the same weight in the output P(k), i.e. we cannot account for the different weighting of the pixels in a straightforward way.
This does not mean we couldn't in principle do the cont fitting with some weighting scheme, but effects this has need to be studied.
Agreed that within a run, i.e. after specifying which kind of weights one wants, things should be treated self-consistently.
There was a problem hiding this comment.
Maybe we should open an issue to discuss how to proceed with this
There was a problem hiding this comment.
I think it's fine as long as we keep the current major modes (i.e. standard or constant weights, not sure if anyone uses ivar-only weights) in working shape which allows us to do analyses similar to previous works.
We could have a discussion if other weightings might be useful, then implement those, and then if they have been tested and lead to improvement I'd be fine with dropping previous modes. In principle, already the change in binning from log to lin can be seen as a λ-dependent change in weighting, at least for e.g. constant weights.
| @@ -11,7 +11,7 @@ ignore=CVS | |||
|
|
|||
| # Add files or directories matching the regex patterns to the blacklist. The | |||
There was a problem hiding this comment.
should we maybe relax a little on e..g. line lengths? here and for yapf? sometimes it's just simpler to allow slightly longer lines and not have as much of trouble with the wrapped lines which can become annoying (e.g. for imports, comments, deeply nested loops)
There was a problem hiding this comment.
There is some margin that we can relax, but then lines that are too long are a pain to read as you have to keep moving the code left and right
There was a problem hiding this comment.
If I recall properly, though, the values we are using are already a bit more relaxed than the default values for pylint
There was a problem hiding this comment.
Ah, didn't read the whole pylintrc. We're using 100 chars, which is ok (the default is ~80 which is a little narrow often enough and going way larger leads to trouble when having 2 files next to each other e.g. in a diff). And indeed if I parse the regexp correctly we're allowing comments and web-addresses to go beyond the line limit (and could add other stuff to there if we figure out there's issues).
Is yapf using the same setup that the linter uses? Or will that produce the default settings or has a seperate config? Iirc there was some way to unify all of this, but I don't remember right now how it worked...
There was a problem hiding this comment.
I guess yapf is using google coding standards with their defaults (as I have not created a config file for that)
There was a problem hiding this comment.
ok, let's just keep it like that for the moment.
|
Please note that this will partially clash with #887, so might be good to have a look which merge order might be less troublesome |
|
I'll review #887 before merging this one. I'll check locally which is the easiest merge order |
This PR adds pylint as an automatic check (only for delta_extraction)
In order to pass the test, some basic linting is performed. Before the PR is ready for review, some bits and pieces must be reorganized in order to avoid code duplication