Enh/motor thrustcurve api #870

Marchma0 · 2025-11-04T18:09:28Z

Pull request type

Code changes (bugfix, features)
Code maintenance (refactoring, formatting, tests)

Checklist

Tests for the changes have been added (if needed)
Lint (black rocketpy/ tests/) has passed locally
All tests (pytest tests -m slow --runslow) have passed locally

Current behavior

#661

New behavior

We added the function to load a motor from the thrustcruve API and we tested it.

Breaking change

No

…e test

Gui-FernandesBR · 2025-11-04T18:22:01Z

I really like this implementation!!

Copilot

Pull Request Overview

This PR adds a new method load_from_thrustcurve_api to the GenericMotor class that allows users to download motor data directly from the ThrustCurve.org API by providing a motor name. This feature simplifies motor initialization by eliminating the need to manually download .eng files.

Adds API integration with ThrustCurve.org to download motor data
Implements a new static method that searches for motors and downloads their .eng files
Includes comprehensive test coverage for the new functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
rocketpy/motors/motor.py	Adds new static method `load_from_thrustcurve_api` with required imports (base64, tempfile, requests) to enable downloading and loading motor data from ThrustCurve API
tests/unit/motors/test_genericmotor.py	Adds test case to verify the new API loading functionality works correctly with Cesaroni M1670 motor data

Copilot · 2025-11-04T18:23:26Z

rocketpy/motors/motor.py

+    @staticmethod
+    def load_from_thrustcurve_api(name: str, **kwargs):


The method signature includes name and **kwargs parameters, but the docstring example shows 'Cesaroni_M1670' format while the test uses 'M1670'. The documentation should clarify the expected input format and provide examples for both manufacturer-prefixed and non-prefixed motor names to avoid user confusion.

Copilot · 2025-11-04T18:23:26Z

rocketpy/motors/motor.py

+            print("No motor found.")
+            return None


Returning None when no motor is found is problematic. This should raise a descriptive exception (e.g., ValueError) with a helpful message suggesting the user verify the motor name format. Returning None can lead to confusing AttributeErrors later when users try to use the returned value.

Suggested change

print("No motor found.")

return None

raise ValueError(

f"No motor found for name '{name}'. "

"Please verify the motor name format (e.g., 'Cesaroni_M1670') and try again."

)

Copilot · 2025-11-04T18:23:27Z

rocketpy/motors/motor.py

+            print("No motor found.")
+            return None


Using print() for user feedback is not a best practice in library code. Since RocketPy doesn't appear to use logging, consider raising an exception instead (as suggested in Comment 2) or using warnings.warn() if you want non-fatal feedback. Print statements make it difficult for users to capture or suppress output programmatically.

Suggested change

print("No motor found.")

return None

raise ValueError(

f"No motor found for name '{name}'. Please check the motor name and try again."

)

Copilot · 2025-11-04T18:23:27Z

rocketpy/motors/motor.py

+        print(f"Motor found: {designation} ({motor['manufacturer']})")
+


Similar to the previous print statement, this informational message should not use print() in library code. If this information is important for users, consider adding it to the motor's metadata or attributes, or use warnings.warn() for non-critical information.

Suggested change

print(f"Motor found: {designation} ({motor['manufacturer']})")

warnings.warn(

f"Motor found: {designation} ({motor['manufacturer']})",

UserWarning,

)

Instead of using warnings.warn(), we replaced the print() with proper logging(in the latest commit). This allows users to control the output via RocketPy’s logging configuration and avoids polluting standard output. The message is now logged at the INFO level in rocketpy/motors/motor.py.

Although you're idea was smart and does make a lot a sense, we currently do not have logging utilities in any other part of this repository.
@phmbressan I'd like to hear your opinion about this also, but I would say the "warning approach" is more familiar to the rest of our code.

Understood. Just made the change to use warnings.warn(). Thanks for the quick feedback.

Copilot · 2025-11-04T18:23:27Z

rocketpy/motors/motor.py

+        )
+        dl_response.raise_for_status()
+        data = dl_response.json()
+


There's no validation that data['results'] is not empty before accessing data['results'][0]. If the API returns an empty results array for the download endpoint, this will raise an IndexError. Add validation similar to line 1946 to check for empty results and provide a clear error message.

Suggested change

if not data.get("results"):

print("No .eng file found for this motor in the ThrustCurve API.")

return None

Copilot · 2025-11-04T18:23:28Z

rocketpy/motors/motor.py

+
+        return motor
+


The motor is created inside the context manager, but the temporary file will be deleted when exiting the with block (due to delete=True). If load_from_eng_file or any subsequent processing tries to access the file after the context manager exits, it will fail. Move the return statement inside the with block or consider using delete=False with manual cleanup if the file needs to persist.

Suggested change

return motor

return motor

Copilot · 2025-11-04T18:23:28Z

rocketpy/motors/motor.py

+        response = requests.get(f"{base_url}/search.json", params={"commonName": name})
+        response.raise_for_status()


The method doesn't document the potential exceptions that can be raised, particularly requests.exceptions.RequestException from network issues or HTTP errors. According to Python best practices and to help users handle errors appropriately, add a 'Raises' section to the docstring documenting requests.exceptions.RequestException and any ValueError exceptions.

Gui-FernandesBR · 2025-11-04T18:28:53Z

I see 2 major problems:

1 - Documentation: we should add a description of how to use the new feature.
2 - Tests: We should mock the API call so we don't exhaust the endpoint whenever we run the tests

…with clean imports

rocketpy/motors/motor.py

Gui-FernandesBR · 2025-11-05T01:33:47Z

rocketpy/motors/motor.py

+            motor_instance = GenericMotor.load_from_eng_file(tmp_path, **kwargs)
+            return motor_instance


noblock:

Suggested change

motor_instance = GenericMotor.load_from_eng_file(tmp_path, **kwargs)

return motor_instance

return GenericMotor.load_from_eng_file(tmp_path, **kwargs)

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

Copilot · 2025-11-05T01:37:33Z

rocketpy/motors/motor.py

+import base64
 import re
+import tempfile
 import warnings
 import xml.etree.ElementTree as ET
 from abc import ABC, abstractmethod
 from functools import cached_property
-from os import path
+from os import path, remove

 import numpy as np
-
+import requests
+import logging


Import order violates PEP 8 conventions. Standard library imports should be grouped together, with third-party imports in a separate group. Move logging (line 12) to the standard library group (between lines 1-8), and move requests (line 11) to join numpy (line 10) in the third-party group.

simply run the command make format and it will solve the issue

Copilot · 2025-11-05T01:37:33Z

rocketpy/motors/motor.py

+                tmp_file.flush()
+                tmp_path = tmp_file.name
+
+


Line 1996 contains only whitespace. Remove this empty line to maintain code cleanliness and consistency with the rest of the codebase.

Suggested change

make format

rocketpy/motors/motor.py

Copilot · 2025-11-05T01:37:34Z

rocketpy/motors/motor.py

+            motor_instance = GenericMotor.load_from_eng_file(tmp_path, **kwargs)
+            return motor_instance
+        finally:
+            # Ensuring the temporary file is removed


For consistency with the codebase comment style, use imperative mood: change 'Ensuring' to 'Ensure'.

Suggested change

# Ensuring the temporary file is removed

# Ensure the temporary file is removed

Copilot · 2025-11-05T01:37:34Z

rocketpy/motors/motor.py

            interpolation_method=interpolation_method,
            coordinate_system_orientation=coordinate_system_orientation,
        )
+


Line 1921 contains trailing whitespace. Remove trailing spaces to maintain code cleanliness.

Suggested change

Copilot · 2025-11-05T01:37:35Z

tests/unit/motors/test_genericmotor.py

 import numpy as np
 import pytest
 import scipy.integrate
+import requests
+import base64


Import order violates PEP 8 conventions. Standard library imports (base64) should come before third-party imports. Move base64 to line 1 before numpy, and group standard library imports together.

Suggested change

import numpy as np

import pytest

import scipy.integrate

import requests

import base64

import base64

import numpy as np

import pytest

import scipy.integrate

import requests

make format

Copilot · 2025-11-05T01:37:35Z

tests/unit/motors/test_genericmotor.py

+        The pytest monkeypatch fixture for mocking.
+    generic_motor : rocketpy.GenericMotor
+        The GenericMotor object to be used in the tests.
+        


Line 227 contains trailing whitespace in the docstring. Remove trailing spaces to maintain code cleanliness.

Suggested change

Copilot · 2025-11-05T01:37:35Z

tests/unit/motors/test_genericmotor.py

+    """
+    Tests the GenericMotor.load_from_thrustcurve_api method with mocked ThrustCurve API responses.
+    Parameters
+    ----------
+    monkeypatch : pytest.MonkeyPatch
+        The pytest monkeypatch fixture for mocking.
+    generic_motor : rocketpy.GenericMotor
+        The GenericMotor object to be used in the tests.
+        


[nitpick] Test docstring should follow the project's NumPy-style docstring format more precisely. The first line should be a brief summary that fits on one line. Consider: 'Tests the GenericMotor.load_from_thrustcurve_api method with mocked API responses.' Move implementation details to a Notes section if needed.

Suggested change

"""

Tests the GenericMotor.load_from_thrustcurve_api method with mocked ThrustCurve API responses.

Parameters

----------

monkeypatch : pytest.MonkeyPatch

The pytest monkeypatch fixture for mocking.

generic_motor : rocketpy.GenericMotor

The GenericMotor object to be used in the tests.

"""Tests the GenericMotor.load_from_thrustcurve_api method with mocked API responses.

Parameters

----------

monkeypatch : pytest.MonkeyPatch

The pytest monkeypatch fixture for mocking.

generic_motor : rocketpy.GenericMotor

The GenericMotor object to be used in the tests.

Notes

-----

This test mocks the ThrustCurve API endpoints 'search.json' and 'download.json'

to provide controlled responses. The .eng file is read locally and its content

is base64-encoded to simulate the API's behavior.

codecov · 2025-11-05T01:41:00Z

Codecov Report

❌ Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.31%. Comparing base (9cf3dd4) to head (d6c5dee).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
rocketpy/motors/motor.py	94.59%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #870      +/-   ##
===========================================
+ Coverage    80.27%   80.31%   +0.03%     
===========================================
  Files          104      104              
  Lines        12769    12805      +36     
===========================================
+ Hits         10250    10284      +34     
- Misses        2519     2521       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…urve_api with exception testing

Monta120 · 2025-11-05T07:42:08Z

@Gui-FernandesBR Thank you for going over the code. I just updated the latest commit to run make format for import/order/style cleanup and added tests for exception handling in load_from_thrustcurve_api.

…(as requested)

… genericmotors.rst file

Add the function to create a motor from the API of thrustcurve and th…

05ab711

…e test

Marchma0 requested a review from a team as a code owner November 4, 2025 18:09

github-project-automation bot added this to LibDev Roadmap Nov 4, 2025

github-project-automation bot moved this to Backlog in LibDev Roadmap Nov 4, 2025

Gui-FernandesBR requested a review from Copilot November 4, 2025 18:20

Gui-FernandesBR changed the base branch from master to develop November 4, 2025 18:20

Copilot AI reviewed Nov 4, 2025

View reviewed changes

Monta120 force-pushed the enh/motor-thrustcurve-api branch from ff57272 to 2770ba2 Compare November 4, 2025 21:32

Improve load_from_thrustcurve_api and test_load_from_thrustcurve_api …

da39fcb

…with clean imports

Monta120 force-pushed the enh/motor-thrustcurve-api branch from 2770ba2 to da39fcb Compare November 4, 2025 21:57

Gui-FernandesBR reviewed Nov 5, 2025

View reviewed changes

rocketpy/motors/motor.py Show resolved Hide resolved

Gui-FernandesBR reviewed Nov 5, 2025

View reviewed changes

Gui-FernandesBR requested review from Monta120, Copilot and phmbressan and removed request for Monta120 November 5, 2025 01:34

Copilot AI reviewed Nov 5, 2025

View reviewed changes

Clean up load_from_thrustcurve_api and improve test_load_from_thrustc…

9fdc704

…urve_api with exception testing

Monta120 added 2 commits November 5, 2025 12:43

Use warnings.warn() in load_from_thrustcurve_api when motor is found …

d123b47

…(as requested)

Added documentation for the load_from_thrustcurve_api method into the…

d6c5dee

… genericmotors.rst file

		@staticmethod
		def load_from_thrustcurve_api(name: str, **kwargs):

-            print("No motor found.")
-            return None
+            raise ValueError(
+                f"No motor found for name '{name}'. "
+                "Please verify the motor name format (e.g., 'Cesaroni_M1670') and try again."
+            )

		print(f"Motor found: {designation} ({motor['manufacturer']})")

-        print(f"Motor found: {designation} ({motor['manufacturer']})")
+        warnings.warn(
+            f"Motor found: {designation} ({motor['manufacturer']})",
+            UserWarning,
+        )

+        if not data.get("results"):
+            print("No .eng file found for this motor in the ThrustCurve API.")
+            return None

		response = requests.get(f"{base_url}/search.json", params={"commonName": name})
		response.raise_for_status()

		motor_instance = GenericMotor.load_from_eng_file(tmp_path, **kwargs)
		return motor_instance

	motor_instance = GenericMotor.load_from_eng_file(tmp_path, **kwargs)
	return motor_instance
	return GenericMotor.load_from_eng_file(tmp_path, **kwargs)

	# Ensuring the temporary file is removed
	# Ensure the temporary file is removed

Uh oh!

Enh/motor thrustcurve api #870

Are you sure you want to change the base?

Enh/motor thrustcurve api #870

Conversation

Marchma0 commented Nov 4, 2025

Pull request type

Checklist

Current behavior

New behavior

Breaking change

Uh oh!

Gui-FernandesBR commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Monta120 Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gui-FernandesBR Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Monta120 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Gui-FernandesBR commented Nov 4, 2025

Uh oh!

Uh oh!

Gui-FernandesBR Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Gui-FernandesBR Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Gui-FernandesBR Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Monta120 Nov 4, 2025 •

edited

Loading

codecov bot commented Nov 5, 2025 •

edited

Loading