Skip to content

new colors branch with clean history #841

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

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

pmocz
Copy link
Member

@pmocz pmocz commented Jul 15, 2025

meant to replace: #786

Todo:

  • update changelog
  • move rest of custom things out of run_stars_extras.f90
  • add new colors options to rsp tests, which used to use old colors?
  • add table caching (for future PR)

@pmocz pmocz changed the title [ci skip] new branch with clean history new colors branch with clean history Jul 15, 2025
@pmocz pmocz requested a review from rsmolec as a code owner July 16, 2025 20:31
Comment on lines 364 to 365
character(len=strlen) :: color_file_names !(max_num_color_files)
integer :: color_num_colors !(max_num_bcs_per_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still use this variable at all, and it really is now scalar instead of array? Or should we just delete it entirely?

Suggested change
character(len=strlen) :: color_file_names !(max_num_color_files)
integer :: color_num_colors !(max_num_bcs_per_file)

@@ -27,7 +27,7 @@ module star_data_def
use eos_def, only: EoS_General_Info
use kap_def, only: Kap_General_Info
use net_def, only: Net_General_Info, other_net_derivs_interface
use colors_def, only: max_num_color_files, max_num_bcs_per_file
use colors_def !, only: max_num_color_files, max_num_bcs_per_file
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're no longer using anything from colors_def, can we not just delete this?

Suggested change
use colors_def !, only: max_num_color_files, max_num_bcs_per_file

If we are using anything, the only clause is useful for tracing where a subroutine etc. came from (because Fortran doesn't have namespacing like other languages).

Comment on lines 276 to 312
! Function values at corners
DO iz = 1, 2
DO iy = 1, 2
DO ix = 1, 2
! Function values
IF (ix == 1) THEN
IF (iy == 1) THEN
IF (iz == 1) THEN
sum = sum + h00_x * h00_y * h00_z * values(ix,iy,iz)
ELSE
sum = sum + h00_x * h00_y * h01_z * values(ix,iy,iz)
END IF
ELSE
IF (iz == 1) THEN
sum = sum + h00_x * h01_y * h00_z * values(ix,iy,iz)
ELSE
sum = sum + h00_x * h01_y * h01_z * values(ix,iy,iz)
END IF
END IF
ELSE
IF (iy == 1) THEN
IF (iz == 1) THEN
sum = sum + h01_x * h00_y * h00_z * values(ix,iy,iz)
ELSE
sum = sum + h01_x * h00_y * h01_z * values(ix,iy,iz)
END IF
ELSE
IF (iz == 1) THEN
sum = sum + h01_x * h01_y * h00_z * values(ix,iy,iz)
ELSE
sum = sum + h01_x * h01_y * h01_z * values(ix,iy,iz)
END IF
END IF
END IF
END DO
END DO
END DO
Copy link
Contributor

Choose a reason for hiding this comment

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

This (and the other loops) look like an elaborate way of writing

Suggested change
! Function values at corners
DO iz = 1, 2
DO iy = 1, 2
DO ix = 1, 2
! Function values
IF (ix == 1) THEN
IF (iy == 1) THEN
IF (iz == 1) THEN
sum = sum + h00_x * h00_y * h00_z * values(ix,iy,iz)
ELSE
sum = sum + h00_x * h00_y * h01_z * values(ix,iy,iz)
END IF
ELSE
IF (iz == 1) THEN
sum = sum + h00_x * h01_y * h00_z * values(ix,iy,iz)
ELSE
sum = sum + h00_x * h01_y * h01_z * values(ix,iy,iz)
END IF
END IF
ELSE
IF (iy == 1) THEN
IF (iz == 1) THEN
sum = sum + h01_x * h00_y * h00_z * values(ix,iy,iz)
ELSE
sum = sum + h01_x * h00_y * h01_z * values(ix,iy,iz)
END IF
ELSE
IF (iz == 1) THEN
sum = sum + h01_x * h01_y * h00_z * values(ix,iy,iz)
ELSE
sum = sum + h01_x * h01_y * h01_z * values(ix,iy,iz)
END IF
END IF
END IF
END DO
END DO
END DO
! Function values at corners
sum = sum + h00_x * h00_y * h00_z * values(1,1,1)
sum = sum + h00_x * h00_y * h01_z * values(1,1,2)
sum = sum + h00_x * h01_y * h00_z * values(1,2,1)
sum = sum + h00_x * h01_y * h01_z * values(1,2,2)
sum = sum + h01_x * h00_y * h00_z * values(2,1,1)
sum = sum + h01_x * h00_y * h01_z * values(2,1,2)
sum = sum + h01_x * h01_y * h00_z * values(2,2,1)
sum = sum + h01_x * h01_y * h01_z * values(2,2,2)

I think you can rather loop over this (and possibly the next blocks too) if h??_? become an array h(2,2,3).

In Fortran, you can declare different array bounds if you want e.g.

real(dp) :: my_array(0:3)

if you want e.g. lower bound 0 and upper bound 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, to loop, values and d{x,y,z}_values would also have to become an array, I think of shape (4,2,2,2).

Comment on lines 500 to 508
min_dist = ABS(x_val - x_grid(1))
i_x = 1
DO i = 2, SIZE(x_grid)
dist = ABS(x_val - x_grid(i))
IF (dist < min_dist) THEN
min_dist = dist
i_x = i
END IF
END DO
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the Fortran instrinsic MINLOC here? Something like

Suggested change
min_dist = ABS(x_val - x_grid(1))
i_x = 1
DO i = 2, SIZE(x_grid)
dist = ABS(x_val - x_grid(i))
IF (dist < min_dist) THEN
min_dist = dist
i_x = i
END IF
END DO
i_x = MINLOC(ABS(x_val - x_grid))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, I have changed this. Thank you

FUNCTION h00(t) RESULT(h)
REAL(dp), INTENT(IN) :: t
REAL(dp) :: h
h = 2.0_dp*t**3 - 3.0_dp*t**2 + 1.0_dp
Copy link
Contributor

Choose a reason for hiding this comment

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

For bit-for-bit reproducibility, use pow or powN from our math_lib, or just write the multiplications out explicitly. E.g.

Suggested change
h = 2.0_dp*t**3 - 3.0_dp*t**2 + 1.0_dp
h = 2.0_dp*t*t*t - 3.0_dp*t*t + 1.0_dp

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you I did not know that

@rhdtownsend
Copy link
Contributor

Many of the DEALLOCATE statements at the end of routines are superfluous -- allocatable arrays that are allocated inside a routine are automatically deallocated on exit (unless they are dummy arguments).

self.ax.set_ylim(self.ylim)
plt.tight_layout()

def parse_inlist_file(self, inlist_path="../inlist_colors"):
Copy link
Member Author

Choose a reason for hiding this comment

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

This function is already defined above in the class? Is this re-definition needed/used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a mistake as I was working to remove pandas dependency. I have changed this now.

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