Skip to content

Larger Spectral Norm Benchmark Case#28895

Open
insertinterestingnamehere wants to merge 1 commit into
chapel-lang:mainfrom
insertinterestingnamehere:bench
Open

Larger Spectral Norm Benchmark Case#28895
insertinterestingnamehere wants to merge 1 commit into
chapel-lang:mainfrom
insertinterestingnamehere:bench

Conversation

@insertinterestingnamehere

Copy link
Copy Markdown
Contributor

This adds a larger spectral norm benchmark case since the existing problem sizes result in near trivial running times.

@insertinterestingnamehere

Copy link
Copy Markdown
Contributor Author

One potentially open question on this one: is the "submitted" directory the right place to put this? It's running on a submitted code, but I wasn't entirely sure.

@jabraham17

Copy link
Copy Markdown
Member

If you don't want to edit the submitted directory, I believe test/studies/shootout/spectral-norm/bradc/spectralnorm-blc.chpl is an identical perf test that is not in "submitted". Any "submitted" test likely has a copy somewhere else in our test tree

@insertinterestingnamehere

Copy link
Copy Markdown
Contributor Author

FWIW, I don't mind editing the submitted directory. I just wasn't sure what made sense in terms of the current directory structure and wanted to confirm. Really whatever is fine with me.

@jabraham17

Copy link
Copy Markdown
Member

I'd probably edit the other benchmark, not the submitted one.

Also, is the intention of this PR to add a perf graph for the larger data size, or just run it with a larger size?

@insertinterestingnamehere

Copy link
Copy Markdown
Contributor Author

If I understood right, I think Brad wanted to add another graph to maintain continuity for the old one. I just want at least one that takes a less trivial amount of time. All of the spectral norm benchmarks run on relatively small problem sizes right now.

@insertinterestingnamehere

Copy link
Copy Markdown
Contributor Author

If it were up to me, I'd change all of them to just run on the bigger problem size and drop the size loop that's present, but I'll definitely defer to you all on what's wanted there.

@insertinterestingnamehere

Copy link
Copy Markdown
Contributor Author

Alright, just switched where the new benchmark case was added.

@insertinterestingnamehere

Copy link
Copy Markdown
Contributor Author

Although, looking at this again, I think this size may be slightly too big for smaller hardware. I'll scale it down a little bit.

@insertinterestingnamehere

Copy link
Copy Markdown
Contributor Author

I think this is good to go on my end. Up to you all what you want to do here though.

@bradcray

Copy link
Copy Markdown
Member

Catching up with this, which I hadn't noticed until a mail mentioned it today.

@insertinterestingnamehere : I think that in order to get a graph for this, you'll need to add a .graph file as well and to add that graph to test/GRAPHFILES. There's a bit of information about that here, though copy-pasting an existing one is a reasonable practice as well and will probably get you there. We don't need to hold up the PR over this, as I think the test system is happy to run things that aren't graphed (?), and I don't know how much you are or are not relying on the graphs.

Sorry not to have seen this earlier, but I'd probably add the new problem size to the submitted rather than blc version, though either is fine. The reason is that I consider the -blc versions to be ones that I might continue to change and experiment over time, whereas the submitted ones are going to remain the same as long as those are the versions we've got submitted on the actual site. It's important to me to continue to run the site's problem size in our nightly testing (which is why I didn't want to just bump up the problem size), but I don't mind adding additional sizes as well.

@bradcray

Copy link
Copy Markdown
Member

(that said, using a larger problem size with the -blc version is fine as well… I just don't promise that it will stay in sync with the submitted version over time).

Signed-off-by: Ian Henriksen <iandhenriksen@gmail.com>

@jabraham17 jabraham17 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Noting that the new graphfile needs to be added to test/GRAPHFILES, otherwise I think this is pretty much ready to merge

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.

3 participants