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

Adding the assemble_archive_from_position function into the docs #380

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

jessicapilling
Copy link
Collaborator

I have added in a section to the 'Observations of Messier 51 from many telescope archives' notebook that demonstrates how to use the assemble_archive_from_position function

@DavidT3
Copy link
Owner

DavidT3 commented Jan 30, 2025

Hi Jess - nice work on this. I'm just going through it and I'll add any things I spot that need changing. First thing is in the writing in the introduction for the one line solution - 'excecutes'

@DavidT3
Copy link
Owner

DavidT3 commented Jan 30, 2025

"all available missions within DAXA, perform the" -> "all available missions within DAXA, calls the"

@DavidT3
Copy link
Owner

DavidT3 commented Jan 30, 2025

"return a list of filtered BaseMission objects which have observations after the filtering." <- the wording here is a little clumsy I think

@DavidT3
Copy link
Owner

DavidT3 commented Jan 30, 2025

I think when it comes to this bit:

"So we can finally run the assemble_archive_from_positions() function with our inputs:"

You should reiterate that if you ran it without the missions input it would just use all of them.

@DavidT3
Copy link
Owner

DavidT3 commented Jan 30, 2025

Maybe you should also mention the other function as well, that just searches without defining an archive, I think that will be quite useful if you're just exploring and don't want to make anything permanent yet.

Copy link
Owner

@DavidT3 DavidT3 left a comment

Choose a reason for hiding this comment

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

I altered a couple of things, which included adding the daxa.mission.tools section that you added to the API documentation files, otherwise it wouldn't have been included (a reminder that whenever you add an entirely new sub-module or sub-module file, you need to look at the RST files and add a new section - I can demo this for you if you need!)

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