Skip to content

Add n_pred parameter to Lightcurve.plot() to control prediction grid size#150

Draft
Copilot wants to merge 3 commits intomainfrom
copilot/modify-lightcurve-plot-n-prediction-points
Draft

Add n_pred parameter to Lightcurve.plot() to control prediction grid size#150
Copilot wants to merge 3 commits intomainfrom
copilot/modify-lightcurve-plot-n-prediction-points

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 1, 2026

Lightcurve.plot() hard-coded 10,000 prediction points for the fine time grid, causing memory issues for 2D light curves. This adds a n_pred parameter (default 1000) to make the grid size user-configurable.

Changes

  • Lightcurve.plot() signature: Added n_pred=1000 parameter
  • Grid construction: Replaced torch.linspace(..., 10000) with torch.linspace(..., n_pred)
  • Docstring: Added numpydoc entry for n_pred (wrapped to ≤88 chars)
  • Input validation: n_pred is validated early — rejects booleans, non-integers, and values < 2; accepts numpy integers via np.integer
  • ndim > 2 fix: NotImplementedError is now raised before x_raw/x_fine_raw are computed, preventing a confusing UnboundLocalError on higher-dimensional data
  • Tests: Added TestPlotNPred with 8 tests covering 1D/2D acceptance, invalid value rejection (float, bool, zero, negative, one), and numpy integer acceptance

Usage

lc.plot()               # 1000 points (default)
lc.plot(n_pred=200)     # faster / lower memory
lc.plot(n_pred=5000)    # higher resolution

The default changes from 10,000 → 1,000, which also reduces default memory consumption.

Agent-Logs-Url: https://github.com/ICSM/pgmuvi/sessions/52de7205-ab74-4003-aec4-1ab528fb8cc2

Co-authored-by: sundarjhu <15619959+sundarjhu@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a user-configurable n_pred parameter to Lightcurve.plot() to control the size of the fine prediction time grid, reducing default memory usage (especially for 2D light curves).

Changes:

  • Extend Lightcurve.plot() signature with n_pred=1000.
  • Use n_pred instead of a hard-coded 10,000 steps when building x_fine_raw.
  • Document n_pred in the plot() numpydoc docstring.

Comment thread pgmuvi/lightcurve.py Outdated
Comment on lines +9620 to +9621
for plotting. Lower values reduce memory usage and speed up plotting,
especially for 2D light curves. Default is 1000.
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

The new docstring lines for n_pred appear to exceed the repo’s configured 88-character line length (Black/Ruff), which will trigger E501. Please wrap the description across multiple lines to keep each line within the limit.

Suggested change
for plotting. Lower values reduce memory usage and speed up plotting,
especially for 2D light curves. Default is 1000.
for plotting. Lower values reduce memory usage and speed up
plotting, especially for 2D light curves. Default is 1000.

Copilot uses AI. Check for mistakes.
Comment thread pgmuvi/lightcurve.py
Comment on lines +9592 to +9594
mcmc_samples=False,
n_pred=1000,
**kwargs,
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

n_pred is passed directly to torch.linspace(..., steps=n_pred). To avoid cryptic runtime errors (e.g., when n_pred is 0/1, negative, non-int, or a NumPy scalar), please validate n_pred early (type + >= 2) and raise a clear ValueError.

Copilot uses AI. Check for mistakes.
Comment thread pgmuvi/lightcurve.py
Comment on lines 9670 to 9678
if self.ndim == 1:
x_raw = self.xdata
elif self.ndim == 2:
x_raw = self.xdata[:, 0]
# y_raw = self.ydata

# creating array of 10000 test points across the range of the data
x_fine_raw = torch.linspace(x_raw.min(), x_raw.max(), 10000)
# creating array of test points across the range of the data
x_fine_raw = torch.linspace(x_raw.min(), x_raw.max(), n_pred)

Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

For self.ndim > 2, x_raw is never assigned but x_fine_raw = torch.linspace(x_raw.min(), ...) is still executed, so calling plot() on higher-dimensional lightcurves will raise UnboundLocalError before hitting the intended NotImplementedError. Please move the ndim guard before computing x_fine_raw (or define x_raw for all supported cases).

Copilot uses AI. Check for mistakes.
Comment thread pgmuvi/lightcurve.py
Comment on lines +9587 to +9594
def plot(
self,
ylim=None,
yscale="auto",
show=True,
mcmc_samples=False,
n_pred=1000,
**kwargs,
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

n_pred changes plot()’s public API and default behavior (10,000 → 1,000). There are existing plot tests, but none exercise the new parameter. Please add a small unit test that plot(show=False, n_pred=...) works for both 1D and 2D, and (once validation is added) that invalid n_pred values raise a clear error.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@sundarjhu sundarjhu left a comment

Choose a reason for hiding this comment

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

Looks good to me. @copilot, please provide your review.

@sundarjhu
Copy link
Copy Markdown
Collaborator

@copilot, please implement all your suggestions.

Copilot AI and others added 2 commits May 1, 2026 19:27
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 1, 2026

@copilot, please implement all your suggestions.

Done in 8d297b1. Implemented all four suggestions:

  1. Docstring line length — wrapped the n_pred description to stay within 88 chars.
  2. n_pred validation — raises ValueError for non-integer types (including bool), numpy scalars are accepted via np.integer, and values < 2 are rejected.
  3. ndim > 2 UnboundLocalError fix — raised NotImplementedError early (before entering the with torch.no_grad() block and before x_raw / x_fine_raw are computed), so the error is clean and deterministic.
  4. Unit tests — added TestPlotNPred with 8 tests covering: 1D acceptance, 2D acceptance, float rejection, bool rejection, zero/negative/one rejection, and numpy integer acceptance.

Copilot AI requested a review from sundarjhu May 1, 2026 19:29
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.

4 participants