Skip to content

Conversation

@Luismiguelvillar
Copy link
Member

@Luismiguelvillar Luismiguelvillar commented Oct 7, 2025

This PR fixes #908 by changing the lists parameters in the config file of Beersheba to Tuples. At the same time it corrects functions which take this parameters by changing their expected input from lists to Tuples.

@gonzaponte
Copy link
Collaborator

Perfect. Let's:

  • rebase this branch onto master (you will need to resolve a conflict)
  • re-write the commit history
    • Capitalize first letter
    • Use an imperative voice
    • Be more specific about the changes. For instance:
      • "alter list to tuples" could benefit from a bit of more information like you are referring to the type hints, and specifically in this corner of the project.
      • "implemented suggested changes" needs a lot of context to know what it is about. Also, there is no need to reference if something was a reviewer suggestion or not. Focus the message on describing the changes implemented in the commit.
      • Your third commit message is pretty good. The "in ic_types.py" bit is unnecessary (an implementation detail), but that's ok.

After that, I'll approve the PR.

@Luismiguelvillar Luismiguelvillar changed the title alter lists to tuples Alter list type to Tuples for sample_width and bin_size in Beersheba. Correct expected type for deconvolve_signal in beersheba.py and deconvolution_input and deconvolve in deconv_functions.py Oct 20, 2025
@Luismiguelvillar Luismiguelvillar changed the title Alter list type to Tuples for sample_width and bin_size in Beersheba. Correct expected type for deconvolve_signal in beersheba.py and deconvolution_input and deconvolve in deconv_functions.py Alter list type to Tuples for sample_width and bin_size in Beersheba. Oct 21, 2025
@Luismiguelvillar
Copy link
Member Author

Perfect. Let's:

  • rebase this branch onto master (you will need to resolve a conflict)

  • re-write the commit history

    • Capitalize first letter

    • Use an imperative voice

    • Be more specific about the changes. For instance:

      • "alter list to tuples" could benefit from a bit of more information like you are referring to the type hints, and specifically in this corner of the project.
      • "implemented suggested changes" needs a lot of context to know what it is about. Also, there is no need to reference if something was a reviewer suggestion or not. Focus the message on describing the changes implemented in the commit.
      • Your third commit message is pretty good. The "in ic_types.py" bit is unnecessary (an implementation detail), but that's ok.

After that, I'll approve the PR.

I have already done it! Do you think this is fine?

@gonzaponte
Copy link
Collaborator

That's great. Just to make sure I didn't convey the wrong message, I didn't mean to say that you should squash everything into a single commit. In this case it's a good approach, but that is not the case in general :).

Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

Better type hints for beersheba-related functions. Good job!

@Luismiguelvillar
Copy link
Member Author

Better type hints for beersheba-related functions. Good job!

Yes! I figured hahaha. Should i continue with "Squash and merge" then?

@gonzaponte
Copy link
Collaborator

You mean merging the PR? Unless it's urgent we tend to have a merger that has not been involved in the PR. So I think @Ian0sborne can merge your PR and you can merge Ian's.

@Luismiguelvillar
Copy link
Member Author

You mean merging the PR? Unless it's urgent we tend to have a merger that has not been involved in the PR. So I think @Ian0sborne can merge your PR and you can merge Ian's.

Perfect! Sounds good!

@Ian0sborne Ian0sborne merged commit fe15ec0 into next-exp:master Oct 22, 2025
1 check passed
@Luismiguelvillar Luismiguelvillar deleted the make_parameters_tuples branch November 26, 2025 12:15
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.

Make sample_width, bin_size, diffusion & drop_dist tuples

3 participants