Skip to content

New tw_stime: bits #168

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

Closed
wants to merge 7 commits into from
Closed

New tw_stime: bits #168

wants to merge 7 commits into from

Conversation

gonsie
Copy link
Member

@gonsie gonsie commented Sep 25, 2019

This adds a new implementation of stime, using the TW_STIME API. It adds a bit field to the stime object. Its use is demonstrated in the ross-matrix-models.

I’ll have to figure out a way to test this and increase coverage, maybe using the matrix models?


If this merge represents a feature addition to ROSS, the following items must be completed before the branch will be merged:

  • Document the feature on the blog (See the website Contributing guide).
    Include a link to your blog post in the Pull Request.
  • Builds should cleanly compile with -Wall and -Wextra.
  • One or more TravisCI tests should be created (and they should pass)
  • Through the TravisCI tests, coverage should increase
  • Test with CODES to ensure everything continues to work

@codecov
Copy link

codecov bot commented Sep 25, 2019

Codecov Report

Merging #168 into develop will increase coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #168      +/-   ##
===========================================
+ Coverage    58.24%   58.26%   +0.02%     
===========================================
  Files           33       33              
  Lines         3561     3563       +2     
===========================================
+ Hits          2074     2076       +2     
  Misses        1487     1487              
Impacted Files Coverage Δ
core/tw-opts.c 23.37% <0.00%> (ø)
core/gvt/mpi_allreduce.c 90.62% <100.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e0f8f7...fe92ea0. Read the comment docs.

gonsie and others added 7 commits September 29, 2020 13:22
make unique names for functions.  if a model can only work with one
tw_stime implementation, it should probably call the functions
directly, thus generating compile errors if the wrong type is being
used.
or reimplement allreduce for the new datatype
@nmcglo
Copy link
Member

nmcglo commented Sep 30, 2020

I'm curious about this - can you briefly explain the benefit that is gained from this bitfield?

@gonsie
Copy link
Member Author

gonsie commented Sep 30, 2020

It's the time class used by LLNL/Ross-matrix-models. The bits field is used to allow for "zero" delay events, where only the bits field is incremented (I think, I did not create the model).

That said, I'm actually reassessing this approach. Integrating the model-specific stime "class" into the ROSS repo is not the right solution, models should directly manage the stime objects, which may change as the model is developed. This PR will probably be closed and I'll find a more model-oriented/CMake-enabled way to change out the stime definition.

@nmcglo
Copy link
Member

nmcglo commented Sep 30, 2020

Ah interesting. I've had some work in the pipeline for a while that hacked together 2 dimensional timestamps in ROSS to eliminate non deterministic event ties. There may be some overlap if I'm reading things right.

Basically turning tw_stime into a struct of two doubles was what I ended up doing. Hadn't made any pull request since it required major adjustments to how GVT was computed (and caused a small hit to performance) but was able to deterministically process a model specifically designed to create as many event ties as possible whereas the current ROSS version couldn't.

@gonsie
Copy link
Member Author

gonsie commented Sep 30, 2020

Is the model open source? Can the TW_STIME API be used to implement the stime you describe?

@nmcglo
Copy link
Member

nmcglo commented Sep 30, 2020

It's in a branch of my personal ROSS fork at the moment and technically public. Was planning on expanding it to make a simulation performance study on it in the future but have been sidetracked with finishing my thesis - stupid thesis.

I believe I looked into the API at the time and the only thing that would be problematic is potentially the ADD definition. But it may be a non issue.

@nmcglo
Copy link
Member

nmcglo commented Sep 30, 2020

Er and the model I used to stress test it: eties needs no knowledge of the changes by design. As far as any model is concerned, the offset time value when creating an event remains one dimensional.

@gonsie gonsie mentioned this pull request Oct 6, 2020
1 task
@gonsie gonsie closed this Oct 6, 2020
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