{"action":"created","issue":{"url":"https://api.github.com/repos/openmc-dev/openmc/issues/1101","repository_url":"https://api.github.com/repos/openmc-dev/openmc","labels_url":"https://api.github.com/repos/openmc-dev/openmc/issues/1101/labels{/name}","comments_url":"https://api.github.com/repos/openmc-dev/openmc/issues/1101/comments","events_url":"https://api.github.com/repos/openmc-dev/openmc/issues/1101/events","html_url":"https://github.com/openmc-dev/openmc/pull/1101","id":372129943,"node_id":"MDExOlB1bGxSZXF1ZXN0MjI0NDIxMDE2","number":1101,"title":"Move most tally filters to C++","user":{"login":"smharper","id":4622797,"node_id":"MDQ6VXNlcjQ2MjI3OTc=","avatar_url":"https://avatars3.githubusercontent.com/u/4622797?v=4","gravatar_id":"","url":"https://api.github.com/users/smharper","html_url":"https://github.com/smharper","followers_url":"https://api.github.com/users/smharper/followers","following_url":"https://api.github.com/users/smharper/following{/other_user}","gists_url":"https://api.github.com/users/smharper/gists{/gist_id}","starred_url":"https://api.github.com/users/smharper/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/smharper/subscriptions","organizations_url":"https://api.github.com/users/smharper/orgs","repos_url":"https://api.github.com/users/smharper/repos","events_url":"https://api.github.com/users/smharper/events{/privacy}","received_events_url":"https://api.github.com/users/smharper/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-10-19T22:00:04Z","updated_at":"2018-10-23T02:20:01Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/openmc-dev/openmc/pulls/1101","html_url":"https://github.com/openmc-dev/openmc/pull/1101","diff_url":"https://github.com/openmc-dev/openmc/pull/1101.diff","patch_url":"https://github.com/openmc-dev/openmc/pull/1101.patch"},"body":"With this PR, nearly all of the tally filter implementation is handled by C++.  I didn't touch `EnergyFilter`, its subclass `EnergyoutFilter`, or `DelayedGroupFilter` because I assume they will be trouble due to how they're used in subroutines like `score_fission_eout`.\r\n\r\nThe implementation for each of the filters is pretty short, so I've decided to try leaving them as header-only.  I think it can be a bit of a pain to constantly have to search both .cpp and .h files for implementation, and that split doesn't feel worth it for these filters which usually have far less than 100 LoC of actual implementation.  Let me know if you guys disagree.\r\n\r\nOne nice benefit of this PR is that it allows us to remove a bunch of geometry-related Fortran interoperability boilerplate.  Particularly good examples of this are the distribcell setup code and the mesh binning code.\r\n\r\nNote that a big chunk of C-API code touched by this PR is not covered by our test suite.  I made my own one-off tests and I believe I've checked all the C-API functions, but I do still request a careful review of those bits.\r\n\r\nOn that note, @paulromano, how would you feel about cutting back the scope of the C-API a bit in terms of tally filters?  Consider functions like `openmc_zernike_filter_set_params`.  I can't think of many use cases for changing a Zernike filter's parameters midway through a run so I guess the primary use case for functions like this is as an alternative to XML-based input.  But I remember you said recently that we should probably be pushing the C++ API as the XML alternative which makes a lot of sense to me."},"comment":{"url":"https://api.github.com/repos/openmc-dev/openmc/issues/comments/432062406","html_url":"https://github.com/openmc-dev/openmc/pull/1101#issuecomment-432062406","issue_url":"https://api.github.com/repos/openmc-dev/openmc/issues/1101","id":432062406,"node_id":"MDEyOklzc3VlQ29tbWVudDQzMjA2MjQwNg==","user":{"login":"smharper","id":4622797,"node_id":"MDQ6VXNlcjQ2MjI3OTc=","avatar_url":"https://avatars3.githubusercontent.com/u/4622797?v=4","gravatar_id":"","url":"https://api.github.com/users/smharper","html_url":"https://github.com/smharper","followers_url":"https://api.github.com/users/smharper/followers","following_url":"https://api.github.com/users/smharper/following{/other_user}","gists_url":"https://api.github.com/users/smharper/gists{/gist_id}","starred_url":"https://api.github.com/users/smharper/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/smharper/subscriptions","organizations_url":"https://api.github.com/users/smharper/orgs","repos_url":"https://api.github.com/users/smharper/repos","events_url":"https://api.github.com/users/smharper/events{/privacy}","received_events_url":"https://api.github.com/users/smharper/received_events","type":"User","site_admin":false},"created_at":"2018-10-23T02:20:01Z","updated_at":"2018-10-23T02:20:01Z","author_association":"CONTRIBUTOR","body":"After implementing it, I agree that moving the implementation to the .cpp files was the right thing to do.  In addition to improving readability, it isolates a lot of headers to smaller scopes.  It raised the compile time a bit so we might want to consider a unity build, but I'll leave that for another day.\r\n\r\nThere's a few line comments still not addressed with that last round of commits.  I've commented on them and I'll get around to them soon."}}
{"actor":{"display_login":"smharper"}}