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

First version of SITCOM-1593 notebook, ready for review #148

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

nsevilla
Copy link
Collaborator

No description provided.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@nsevilla nsevilla requested a review from b1quint November 21, 2024 16:51
@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #1.    %load_ext lab_black

Hi Nacho - Unfortunately, the DM folks are removing lab_black from RSP. So some people might have problems running the notebook with this. Can you remove it, please?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Instead of EfdClient , I recommend you use makeEfdClient . This will create an efd client independently if you are running this notebook at the summit or at USDF.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #3.    n_actuators = 156

This is a constant. I would move it to the same cell together with the imports with all letters capital N_ACTUATORS .


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #4.    primary_FA_error = [

If might be more compact to write something like:

primary_FA_error = [f"primaryCylinderFollowingError{i}" for i in range(n_actuators)"]

You can even consider adding the [...] directly to the query to get data from the Efd.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used your proposed syntax.

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #7.    df_primary_FA_error = await client.select_time_series(

Instead of using await client.select_time_series consider using getEfdData which you have imported. The function does basically the same without needing the await.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I think I copy-pasted something very old because many of the options here I have been using in my more recent notebooks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #1.    n_bins = 30

See that n_bins is repeated here. I would delete this one once the other is setup earlier in the notebook.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #9.    for j in range(n_actuators):

Add space before this line


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #28.    bin_centers = (bin_edges[:-1] + bin_edges[1:]) / 2

Add space before this line.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #1.    from collections import defaultdict

I would move all these imports to the import cell in the beginning of the notebook.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #25.        fadata_min["FA"].append(f"Force Actuator {i}")

I recommend grouping the code here. Something like:

for ...
    (optional empty line)
    fadata_mean["elevation"].append(bin_centers)
    fadata_mean["force error"].append(fa_mean[i])
    fadata_mean["FA"].append(f"Force Actuator {i}")
    
    fadata_max["elevation"].append(bin_centers)
    fadata_max["force error"].append(fa_max[i])
    fadata_max["FA"].append(f"Force Actuator {i}")

Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I don't quite understand this comment. How is this any different to what is there already? There is a for loop filling in all these dictionaries as you suggest.

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #6.    max_actuators = np.argmax(df_primary_FA_error_resampled_max, axis=1)

It seems that this is only used in a plot a few cells below. Maybe bring line 6 and 7 closer to their associated plot so it is easier to realize where the data is coming from?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

I wonder if having semilog plot would make it easier to visualize... Maybe add horizontal grid lines?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

This plot looks awesome! It is exactly what I was thinking. We can see quickly how FA 67 is different from the others.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great!

@@ -0,0 +1,514 @@
{
Copy link
Member

@b1quint b1quint Nov 26, 2024

Choose a reason for hiding this comment

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

Line #16.    for i in range(n_actuators):

The only detail to change here would be the parameter you are using to identify the force actuators. Right now, you are using the index. It would be better to have the actuator id. If you print the FATable variable, you will see something like:

ForceActuatorData(index=42, actuator_id=143, x_position=1.60401001, y_position=-3.692780029, z_position=-2.158743, actuator_type=<FAType.SAA: 0>, subnet=<FASubnet.C: 3>, address=3, orientation=<FAOrientation.NA: 0>, x_index=None, y_index=None, z_index=42, s_index=None, near_neighbors=[140, 139, 134, 133, 138, 142], far_neighbors=[139, 140, 142, 138, 134, 133, 135, 141, 128, 137, 132, 129]), 

The actuator_id is what we should display.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed! Thanks for catching this, I didn't know of this subtle detail. In fact, it led me to modifying the histogram before it as well.

@b1quint
Copy link
Member

b1quint commented Dec 16, 2024

Hi Nacho, could you squash your last commits with the previous one(s)? I ask because the outputs remain in the history line forever if you just keep another commit, and they increase the size of the repository.

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