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

Fix dynamic alloc #38

Closed
wants to merge 57 commits into from
Closed

Conversation

hellkite500
Copy link
Member

Refactors the allocation of Q to ensure the correct size for iterating over the histogram size.

Also fixes an accumulation bug where Q was not previously accounting for the time delayed contributions.

Removes hard coded limits and issues warnings when users supply values that may violate original model assumptions.

A few code cleanup/refactors for simplifying and optmization loops.

A few other small bug fixes and comments added.

Note this PR is dependent on #37. It is branched from this commit on the PR branch: cfbdec5

…xing accidental comment associated w/this file
…for printing out updates relevant to calibratable parameters.
@PhilMiller
Copy link
Contributor

Given that #37 is a big functional change that's still in draft mode, would it make sense to

Comment on lines +2 to +7
CC = gcc -g

# define any compile-time flags
# for newer compiler, the right hand side is not needed...
# ... for instance, remove the flags if using gnu/10.1.0 on Cheyenne.
CFLAGS =
CFLAGS = -fsanitize=address
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best not to commit changes like this back to the shared repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

I put this here intentionally since this makefile is used to build the executable that is run under the CI test runner. Since this saga started with a weird memory issue, I thought it might be helpful to have this as the default used to build and test the code for future changes.

@PhilMiller
Copy link
Contributor

Incidentally, the test/main_unit_test_bmi.c here is a decent illustration of the things that I want to capture in a BMI module usage/conventions checking utility. So, I'm glad I came to look.

@hellkite500
Copy link
Member Author

As far as merge/integration is concerned, I'm happy to rebase this again and reorganize the commits as appropriate/needed. I'm not sure a PR against the feature branch of #37 would help significantly, based on discussions with @Ben-Choat I think that work is nearly ready to merge, and that might complicate things. As long as this doesn't go stale, I can rebase on master once the feature is complete.

@hellkite500 hellkite500 linked an issue Sep 21, 2023 that may be closed by this pull request
…y+num_time_delay_histo_ords, instead of just num_time_dealy_histo_ords. Also edited a print statement, and an error to provide assistance to user about how to fix error.
…e Q differently depending on if stand alon emode or not. Seeing discrepancies between master stand_alone results and current stand_alone results. Differences exist before these edits.
@hellkite500 hellkite500 mentioned this pull request Jan 2, 2024
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.

Buffer overwrite in topmodel initialization
3 participants