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

Creating Geometric Model #195

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Creating Geometric Model #195

merged 3 commits into from
Jan 12, 2024

Conversation

Moanalengkeek
Copy link
Collaborator

Description

Summary of changes

Replaces pull request #192 as that one had problems with creating a pull request from a fork.

See #192 for further information

Copy link

github-actions bot commented Jan 12, 2024

Overall Coverage

Coverage Report
FileStmtsMissCoverMissing
paseos
   __init__.py33197%49
   paseos.py1441292%67–68, 145–146, 203, 218, 236, 245, 263, 294, 302–305
paseos/activities
   activity_manager.py43393%46, 74, 157
   activity_processor.py58198%121
   activity_runner.py621084%80–84, 104–113, 122–125
paseos/actors
   actor_builder.py1853283%23–29, 32, 222–224, 251–253, 298–307, 328–335, 474, 531, 559, 571–575, 582–583, 590–591
   base_actor.py1281886%82, 104, 120, 167, 207–209, 230, 239, 256, 277, 283–286, 292, 307, 362
   ground_station_actor.py15380%47–53
   spacecraft_actor.py61198%123
paseos/central_body
   central_body.py65592%68, 177–178, 186–187
   is_in_line_of_sight.py581279%106–123, 167, 187
   mesh_between_points.py35294%64, 72
   sphere_between_points.py26869%35–38, 48–51, 63–64
paseos/communication
   get_communication_window.py23291%38, 64
paseos/geometric_model
   geometric_model.py281643%32–38, 47–79, 92–93, 101–102
paseos/power
   charge_model.py16194%49
   discharge_model.py7271%22, 34
paseos/tests
   activity_test.py57395%90, 93–94
   communication_window_test.py49198%176
   eclipse_test.py10190%20
   import_test.py6183%13
   init_test.py8188%16
   line_of_sight_test.py62494%143–146
   mesh_test.py119397%118, 136, 144
   thermal_model_test.py30197%61
   visualization_test.py18194%28
paseos/utils
   check_cfg.py611870%31, 40, 46, 60–62, 67, 70–72, 75–77, 80–82, 98, 103
   operations_monitor.py71396%127–130
paseos/visualization
   animation.py18667%27–30, 35, 44
   plot.py9367%29–32
   space_animation.py2152389%108–109, 124, 147–148, 182, 206–213, 219, 329, 352–354, 359, 368–369, 373, 409, 415–416, 447, 461–472
TOTAL218719891% 

Tests Skipped Failures Errors Time
36 0 💤 0 ❌ 0 🔥 59.384s ⏱️

@GabrieleMeoni GabrieleMeoni mentioned this pull request Jan 12, 2024
1 task
@GabrieleMeoni GabrieleMeoni requested a review from gomezzz January 12, 2024 14:07
Copy link
Collaborator

@GabrieleMeoni GabrieleMeoni left a comment

Choose a reason for hiding this comment

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

I think this is fine.

@GabrieleMeoni GabrieleMeoni merged commit c7a9030 into main Jan 12, 2024
3 checks passed
@GabrieleMeoni GabrieleMeoni deleted the geometric_model branch January 12, 2024 14:11
Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

@GabrieleMeoni I put some comments here to elaborate, also note there are 0 tests in this PR

paseos/actors/actor_builder.py Show resolved Hide resolved
Args:
actor (SpacecraftActor): Actor to update.
mass (float): Mass of the spacecraft in kg.
vertices (list): List of all vertices of the mesh in terms of distance (in m) from origin of body frame.
Copy link
Collaborator

Choose a reason for hiding this comment

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

no usually they are 3d points not just distances

actor: SpacecraftActor, mass: float, vertices=None, faces=None, scale: float = 1
):
"""Define geometry of the spacecraft actor. This is done in the spacecraft body reference frame, and can be
transformed to the inertial/PASEOS reference frame using the reference frane transformations in the attitude
Copy link
Collaborator

Choose a reason for hiding this comment

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

"frane" -> frame (I can't suggest changes because already merged)

triangles. Uses Trimesh to create the mesh from this and the list of vertices.
scale (float): Parameter to scale the cuboid by, defaults to 1.
"""
assert mass > 0, "Mass is > 0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should check that shape of vertices and faces is N,3, and that all entries in faces are < len(vertices)

"""
assert mass > 0, "Mass is > 0"

actor._mass = mass
Copy link
Collaborator

Choose a reason for hiding this comment

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

should check if mass is set already and give a warning because it might be overwritten

geometric_model = GeometricModel(
local_actor=actor, actor_mass=mass, vertices=vertices, faces=faces, scale=scale
)
actor._mesh = geometric_model.set_mesh()
Copy link
Collaborator

Choose a reason for hiding this comment

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

a set_ function usually gets something passed and sets it in an object but see comment on structure in geometric model

Copy link
Collaborator

Choose a reason for hiding this comment

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

the structure should match existing code here

_thermal_model = None

the actor has the models as an attribute. the reason is that (what is completely missing in this PR) the users needs to be able to get info from the actor

actor.get_rotation
actor.get_mesh
actor.get_whatever

to match existing code this functions should be in the actor

this doesn't work if the actor is only a parameter of the model

Copy link
Collaborator

Choose a reason for hiding this comment

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

(some of those functions should also be added)

Copy link
Collaborator

Choose a reason for hiding this comment

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

would rename to spacecraft_body_model and put it in the actor folder. "geometric" is very vague and it could also be the geometry of the central body

Comment on lines +92 to +93
self._actor_moment_of_inertia = self._actor_mesh.moment_inertia
return self._actor_moment_of_inertia
Copy link
Collaborator

Choose a reason for hiding this comment

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

this function doesn't do anything right now, so please raise a NotImplemented error so it's clear that nothing happens

self._actor_mesh = mesh.apply_scale(self.scale)
return self._actor_mesh

@property
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this a property and the other one not?

@gomezzz gomezzz restored the geometric_model branch January 13, 2024 08:40
@gomezzz
Copy link
Collaborator

gomezzz commented Jan 13, 2024

( @Moanalengkeek pls disregard, moving into discussion with Gabriele about paseos integration)

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.

3 participants