Skip to content

Conversation

@jcass11
Copy link
Collaborator

@jcass11 jcass11 commented Sep 27, 2024

Previously the supplemental figure for our local growth analysis showed the RMSE of the power law fit and had a benchmark line for the segmentation error. This update adds distributions for the RMSEs of exponential and linear fits as well.

To do this, some older functions that expected exact column names because only one fit was being used needed to be made bulkier to be more flexible to iterate through multiple models.

New:
RMSE_linearityfit_volume_distribution_density.pdf

Old:
RMSE_linearityfit_volume_distribution_density.pdf

Copy link
Collaborator

@chantelleleveille chantelleleveille left a comment

Choose a reason for hiding this comment

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

NIce! I like it :) very clear!

@jcass11 jcass11 requested a review from cfrick13 September 27, 2024 21:36
@cfrick13
Copy link
Contributor

cfrick13 commented Oct 2, 2024

I have a nit about the previous version of this code. I noticed that t_scale alpha is being recomputed de novo and outside of the main function being used in the local_growth_workflow.py file. Ideally the code would call add_growth_features. fit_tracks_to_model within this code instead so that the function used is identical to what is used on all the data.

image

Copy link
Contributor

@cfrick13 cfrick13 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 there are some changes that would be good to make before this gets merged.

  1. I'd like to see the local_growth_worklfow directly call the function used to get the function fits instead of recomputing it.
  2. I'd like to see global_dataset_filtering only call fit_tracks_to_model instead of the older fit_tracks_time_powerlaw.

I think these changes will keep the code cleaner and make it easier for any adjustments to these workflows to propagate without need for copying.

@kmitcham kmitcham removed their request for review November 1, 2024 15:50
@jcass11
Copy link
Collaborator Author

jcass11 commented Nov 18, 2024

I have a nit about the previous version of this code. I noticed that t_scale alpha is being recomputed de novo and outside of the main function being used in the local_growth_workflow.py file. Ideally the code would call add_growth_features. fit_tracks_to_model within this code instead so that the function used is identical to what is used on all the data.

image

@cfrick13 I'm confused about this comment. In line 73 its not doing a new fit. In lines 55-57 before that its pulling the values out from the dataframe for the parameters that were already calculated during the during the process_full_tracks step with the line df_full = add_growth_features.fit_tracks_to_model(df_full, interval, "power") and just getting out the resulting volume when those parameters are used in a power fit function. The parameters aren't being fit in line 73 or in this general block of code, the previously fit parameters are being accessed from the dataframe and then plugged into the simple power law function to get the resulting volume values out. If you already saw that and I am totally missing your point please let me know.

@jcass11
Copy link
Collaborator Author

jcass11 commented Nov 18, 2024

I think there are some changes that would be good to make before this gets merged.

  1. I'd like to see the local_growth_worklfow directly call the function used to get the function fits instead of recomputing it.
  2. I'd like to see global_dataset_filtering only call fit_tracks_to_model instead of the older fit_tracks_time_powerlaw.

I think these changes will keep the code cleaner and make it easier for any adjustments to these workflows to propagate without need for copying.

  1. I think this is a misunderstanding - the fits are not being recomputed. Check out my reply to your comment!
  2. You're 100% right! Updated the code to work this way.

@jcass11 jcass11 requested a review from cfrick13 November 18, 2024 20:43
@cfrick13
Copy link
Contributor

@cfrick13 I'm confused about this comment. In line 73 its not doing a new fit. In lines 55-57 before that its pulling the values out from the dataframe for the parameters that were already calculated during the during the process_full_tracks

Just to clarify what I meant originally--I was pointing out that (see image below) the power law fit (z) is being recomputed in local_growth_workflow by copying code from another function (lines ~125-141 in add_growth_features.py) rather than calling that function directly to get the fit trajectory, which could be done by adding a return_fit_trajectory argument.

image

Copy link
Contributor

@cfrick13 cfrick13 left a comment

Choose a reason for hiding this comment

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

Approved!

@jcass11
Copy link
Collaborator Author

jcass11 commented Nov 19, 2024

@cfrick13 I'm confused about this comment. In line 73 its not doing a new fit. In lines 55-57 before that its pulling the values out from the dataframe for the parameters that were already calculated during the during the process_full_tracks

Just to clarify what I meant originally--I was pointing out that (see image below) the power law fit (z) is being recomputed in local_growth_workflow by copying code from another function (lines ~125-141 in add_growth_features.py) rather than calling that function directly to get the fit trajectory, which could be done by adding a return_fit_trajectory argument.

image

OOOOOOOOOH yes I totally misunderstood this. Followed up with Chris in person - I think returning the z from the add_growth_features function would also redo the fitting so the actual best thing to do would be to create a new track-trimming helper function that both the local_growth_workflow and add_growth_features function but since this is only used twice I am going to consider it okay for now. Chris and I agreed that it is "good enough" to leave as is but if I find that I end up reusing it again I should definitely go back and make a shared helper function. Thus leaving a note to document that decision here!

@jcass11 jcass11 added this pull request to the merge queue Nov 19, 2024
Merged via the queue into dev with commit 085e0c0 Nov 19, 2024
1 check passed
@jcass11 jcass11 deleted the modelfits branch November 19, 2024 17:56
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