Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR extends get_pointing_table to accept sunpy TimeRange objects by adding type checks and proper start/end extraction, updates the docstring accordingly, and adds a test to verify the new functionality. Sequence diagram for get_pointing_table time_range parameter handlingsequenceDiagram
participant Caller
participant get_pointing_table
Caller->>get_pointing_table: call with time_range
alt time_range is Time
get_pointing_table->>get_pointing_table: extract start, end from Time
else time_range is TimeRange
get_pointing_table->>get_pointing_table: extract start, end from TimeRange
else invalid type
get_pointing_table->>get_pointing_table: raise TypeError
end
Class diagram for updated get_pointing_table function parameter handlingclassDiagram
class get_pointing_table {
+get_pointing_table(source: str, time_range: Time | TimeRange | tuple)
}
class Time {
+start
+end
}
class TimeRange {
+start
+end
}
get_pointing_table --> Time : accepts
get_pointing_table --> TimeRange : accepts
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `aiapy/calibrate/util.py:238` </location>
<code_context>
msg = "time_range must be provided if the source is 'jsoc'"
raise ValueError(msg)
- start, end = time_range
+ if isinstance(time_range, Time):
+ start, end = time_range[0], time_range[-1]
+ elif isinstance(time_range, TimeRange):
+ start, end = time_range.start, time_range.end
</code_context>
<issue_to_address>
Indexing into Time objects may not always yield expected start/end times.
If time_range contains more than two elements, [0] and [-1] may not represent the correct start and end. Please ensure time_range is always two elements, or update the logic to handle cases with more.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if isinstance(time_range, Time):
start, end = time_range[0], time_range[-1]
elif isinstance(time_range, TimeRange):
start, end = time_range.start, time_range.end
=======
if isinstance(time_range, Time):
if len(time_range) != 2:
msg = (
"If time_range is a Time object, it must contain exactly two elements "
"representing the start and end times."
)
raise ValueError(msg)
start, end = time_range[0], time_range[1]
elif isinstance(time_range, TimeRange):
start, end = time_range.start, time_range.end
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `aiapy/calibrate/tests/test_util.py:122` </location>
<code_context>
+@pytest.mark.remote_data
+def test_get_pointing_table_timerange() -> None:
+ t = Time("2021-01-01T00:00:00", scale="utc")
+ table = get_pointing_table("jsoc", time_range=TimeRange(t - 3 * u.h, t + 3 * u.h))
+ assert isinstance(table, QTable)
+ assert table["T_START"].min() == Time("2020-12-31T21:00:00", scale="utc")
+ assert table["T_STOP"].max() == Time("2021-01-01T09:00:00", scale="utc")
+
+
</code_context>
<issue_to_address>
Missing test for invalid TimeRange input types.
Add a test that passes an invalid type to time_range to verify the TypeError is raised as intended.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
daa4119 to
505fd0e
Compare
Contributor
|
Holy copilot |
Member
Author
|
Its the future, might as well embrace it. |
505fd0e to
34b863d
Compare
Member
Author
|
Less code now. |
43eb70e to
14429d5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #359
Summary by Sourcery
Add support for sunpy.time.TimeRange objects in get_pointing_table and update tests accordingly
New Features:
Tests: