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

Expand access to TILES data #325

Merged
merged 11 commits into from
Mar 4, 2022
Merged

Expand access to TILES data #325

merged 11 commits into from
Mar 4, 2022

Conversation

ddobie
Copy link
Collaborator

@ddobie ddobie commented Feb 8, 2022

Fix #320.
Fix #217.

@ddobie ddobie added this to In progress in v3.0.0 and full-survey data via automation Feb 8, 2022
@pep8speaks
Copy link

pep8speaks commented Feb 8, 2022

Hello @ddobie! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-03-04 00:36:41 UTC

@vast-bot
Copy link

vast-bot commented Feb 8, 2022

Hello @ddobie! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-08 05:25:37 UTC

@ddobie ddobie marked this pull request as ready for review February 8, 2022 05:49
@ddobie ddobie requested a review from ajstewart February 8, 2022 05:50
Copy link
Collaborator

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

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

Is it worth a test to check that the stokes parameter is being understood and used correctly when it gets the files?

@ddobie
Copy link
Collaborator Author

ddobie commented Feb 8, 2022

I'm not sure what you mean by that

@ajstewart
Copy link
Collaborator

I'm not sure what you mean by that

Sorry, I was meaning the test that has changed in this PR, the get_files one, it isn't testing that the different stokes files are obtained correctly. I wondered if it was worth adding a parametrize on that test to also add the stokes argument (if it works like that I can't remember - or if there isn't already a stokes test that I have forgotten about).

Comment on lines 1708 to 1713
image_file_fmt = (
"image.i.{}.SB{}.cont"
"image.{}.{}.SB{}.cont"
".taylor.0.restored.fits".format(
row.field, row.sbid
self.settings['stokes'].lower(), row.field, row.sbid
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might be getting confused here, is there now access to stokes V tiles? And not just stokes I?

As my last comment came from this bit where the stokes has been added.

As there is also this test which may need to be sorted out:

def test_init_failure_stokes_v_tiles(self, mocker) -> None:
"""
Tests the initialisation failure of a Query object.
Specifically when Stokes V is requested on tile images.
Args:
mocker: The pytest-mock mocker object.
Returns:
None
"""
with pytest.raises(vtq.QueryInitError) as excinfo:
isdir_mocker = mocker.patch(
'vasttools.query.os.path.isdir',
return_value=True
)
test_dir = '/testing/folder'
query = vtq.Query(
epochs='all-vast',
planets=['Mars'],
base_folder=test_dir,
stokes='v',
use_tiles=True
)
assert str(excinfo.value).startswith(
"Problems found in query settings!"
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm also a tad confused, but I think/hope my latest commit has addressed what you're talking about.

You're right, there's no access to stokes V tiles yet, but I figured I might as well remove the hardcoded stokes I while I'm working on that part of the code so it's there and ready if we do decide to add it in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok so the coded blocks for Stokes V tiles, like what that test I linked to above is checking, should remain for now?

ajstewart
ajstewart previously approved these changes Mar 3, 2022
Copy link
Collaborator

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

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

All good if the Stokes V tiles block does need to remain.

v3.0.0 and full-survey data automation moved this from In progress to Reviewer approved Mar 3, 2022
v3.0.0 and full-survey data automation moved this from Reviewer approved to Review in progress Mar 4, 2022
@ddobie ddobie merged commit 706e8e6 into dev Mar 4, 2022
v3.0.0 and full-survey data automation moved this from Review in progress to Done Mar 4, 2022
@ajstewart ajstewart deleted the iss320 branch March 4, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Enable access to Tiles RMS data Enable noise measurement and forced extraction to Stokes I tiles
4 participants