Conversation
WalkthroughWalkthroughThis update streamlines geometry management in various simulation classes by standardizing the setting of names and module counts through dedicated methods. It removes outdated methods and variables, enhancing code clarity and maintainability. The changes also include updates to the build system to accommodate these modifications and introduce new tests to ensure consistency in geometry naming conventions. Changes
This table summarizes the key files affected by the update, highlighting the nature of the changes and their impact on the project structure and functionality. Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (22)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (21)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
fairroot/geobase/FairGeoSet.cxx (1)
2-2: Update the copyright year to reflect the current year.Consider updating the copyright year to reflect the current year if this file has been modified during the current year.
fuhlig1
force-pushed
the
improve_geo_classes
branch
from
b3d639c to
37b74ed
Compare
|
@coderabbitai generate unit testing code for examples/MQ/pixelDetector/src/PixelGeo.cxx and examples/MQ/pixelDetector/src/PixelGeo.h |
|
Tip For best results, initiate chat on the files or code changes. @fuhlig1, here is the unit testing code for #include <gtest/gtest.h>
#include "PixelGeo.h"
class PixelGeoTest : public ::testing::Test {
protected:
PixelGeo* pixelGeo;
void SetUp() override {
pixelGeo = new PixelGeo();
}
void TearDown() override {
delete pixelGeo;
}
};
TEST_F(PixelGeoTest, ConstructorTest) {
EXPECT_EQ(pixelGeo->GetName(), "Pixel");
EXPECT_EQ(pixelGeo->GetMaxModules(), 10);
}This test suite includes:
Make sure to include Google Test in your project to compile and run this test. If you need further assistance or additional tests, feel free to ask! Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Fetch the contents of PixelGeo.cxx and PixelGeo.h
cat examples/MQ/pixelDetector/src/PixelGeo.cxx
cat examples/MQ/pixelDetector/src/PixelGeo.h
Length of output: 1811 |
|
@coderabbitai, generate unit testing code for examples/MQ/pixelDetector/src/PixelGeo.cxx and examples/MQ/pixelDetector/src/PixelGeo.h. Take also the base class fairroot/geobase/FairGeoSet.cxx and fairroot/geobase/FairGeoSet.h into account |
|
Tip For best results, initiate chat on the files or code changes. @fuhlig1, based on the provided source code for Here's the unit testing code using Google Test: #include <gtest/gtest.h>
#include "PixelGeo.h"
#include "FairGeoSet.h"
class PixelGeoTest : public ::testing::Test {
protected:
PixelGeo* pixelGeo;
FairGeoSet* fairGeoSet;
void SetUp() override {
pixelGeo = new PixelGeo();
fairGeoSet = pixelGeo; // Upcast to base class to test base class functionality
}
void TearDown() override {
delete pixelGeo;
}
};
TEST_F(PixelGeoTest, ConstructorTest) {
// Test the constructor of PixelGeo
EXPECT_EQ(pixelGeo->GetName(), "Pixel");
EXPECT_EQ(pixelGeo->GetMaxModules(), 10);
}
TEST_F(PixelGeoTest, BaseClassConstructorTest) {
// Test the constructor of the base class FairGeoSet via PixelGeo
EXPECT_EQ(fairGeoSet->GetMaxModules(), 10); // Assuming GetMaxModules is a public method in FairGeoSet
}
TEST_F(PixelGeoTest, ModuleManagement) {
// Test module management functions if applicable
int modules[10] = {1, 0, 1, 0, 1, 0, 1, 0, 1, 0};
fairGeoSet->setModules(-1, modules);
int* retrievedModules = fairGeoSet->getModules();
for (int i = 0; i < 10; ++i) {
EXPECT_EQ(retrievedModules[i], modules[i]);
}
}
// Additional tests can be added here to cover more functionalitiesThis test suite includes:
Make sure to include Google Test in your project to compile and run these tests. If you need further assistance or additional tests, feel free to ask! Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Fetch the contents of PixelGeo.cxx, PixelGeo.h, FairGeoSet.cxx, and FairGeoSet.h
cat examples/MQ/pixelDetector/src/PixelGeo.cxx
cat examples/MQ/pixelDetector/src/PixelGeo.h
cat fairroot/geobase/FairGeoSet.cxx
cat fairroot/geobase/FairGeoSet.h
Length of output: 23325 |
dennisklein
left a comment
dennisklein
left a comment
There was a problem hiding this comment.
In general 👍, I put a few comments inline.
There was a problem hiding this comment.
Something went wrong with the whitespace in this file?
fairroot/geobase/FairGeoSet.h
Outdated
| { | ||
| // construct TString starting after the end of the detector name | ||
| // which contains the detector number | ||
| TString modNumber(name(fName.Length(), (name.Length() - fName.Length()))); |
There was a problem hiding this comment.
| TString modNumber(name(fName.Length(), (name.Length() - fName.Length()))); | |
| assert(name.Length() > fName.Length()); | |
| TString modNumber(name(fName.Length(), (name.Length() - fName.Length()))); |
Makes it a bit more readable IMHO, as this is really the assumption you are making, right?
fairroot/geobase/FairGeoSet.h
Outdated
| // construct TString starting after the end of the detector name | ||
| // which contains the detector number | ||
| TString modNumber(name(fName.Length(), (name.Length() - fName.Length()))); | ||
| return (modNumber.Atoi() - 1); |
There was a problem hiding this comment.
Will this always work? e.g. if fName == "blubb" and name == "blubbXXX" or name == "asdf15", let's add test coverage?
Perhaps this function should also check that the prefixes are equal? Otherwise, if fName == "blubb", getModNumInMod("asdfg42") will work, but should it?
There was a problem hiding this comment.
The ASCII files defining the detector geometry need to implement it the way that all elements in the file start with fName followed by a number. I will add code to check this.
There was a problem hiding this comment.
I added two checks which confirm the consistency of the input.
| virtual const char* getModuleName(Int_t); | ||
| virtual const char* getEleName(Int_t); |
There was a problem hiding this comment.
Actually, thinking more about those functions: I think we should go one step further and change the return type and return a string (by-value). Returning a const char * is really dangerous, because those functions look like ordinary getters (edit: Perhaps not getters, what I mean is, they don't make me expect to depend on each other): all results of a previous call become dangling pointers as soon as the next call happens.
There was a problem hiding this comment.
I thought about the same but unfortunately these functions are public and definitely used in user code so we can't change it without breaking the user code.
Is it possible to implement a second function which only has a different return type? If so we can deprecate the old function and internally use the new one.
There was a problem hiding this comment.
Yes, that would be the better approach for a smoother transition.
There was a problem hiding this comment.
We probably should start by seperating the interfaces that should be called and that should be overriden (there's some cool "pattern" name for this, I have forgotten it).
We should deprecate getModuleName so that people stop calling it.
And here's an idea for the "to be called" method.
std::string CallGetModuleName(Int_t x)
{
// getModuleName usually is not thread safe!
static std::mutex mtx;
std::lock_guard lock{mtx};
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
return {getModuleName(x)};
#pragma GCC diagnostic pop
}There was a problem hiding this comment.
Good idea but we should try to get this PR merged and create issues with the still open problems/suggestions.
We should now focus on getting the warnings fixed such that we can create the new FairRoot release.
fuhlig1
force-pushed
the
improve_geo_classes
branch
from
37b74ed to
9e8d932
Compare
fairroot/geobase/FairGeoSet.h
Outdated
| std::string modNameSet{""}; //! name of module | ||
| std::string eleNameSet{""}; //! substring for elements in module |
There was a problem hiding this comment.
Can these be renamed fModNameSet and fEleNameSet?
fuhlig1
force-pushed
the
improve_geo_classes
branch
from
9e8d932 to
41dc0cd
Compare
| const char* getGeomFile() { return geoFile.Data(); } | ||
| Int_t getMaxSectors(void) { return maxSectors; } | ||
| Int_t getMaxModules(void) { return maxModules; } | ||
| void setMaxModules(Int_t _maxMods) { maxModules = _maxMods; } |
There was a problem hiding this comment.
I think, the general naming convention was about CamelCase, right?
Please let's at least use it for new APIs?
| void setMaxModules(Int_t _maxMods) { maxModules = _maxMods; } | |
| void SetMaxModules(Int_t _maxMods) { maxModules = _maxMods; } |
There was a problem hiding this comment.
Correct. I kept the old convention for the geobase area since this was inherited from Hades. I agree that we should change this but I would like to do it in a second step. There are many more issues with this class which I would like to fix.
The main intention of this PR was to get rid of the sprintf usage. I would like to limit the scope of this PR to this.
There was a problem hiding this comment.
I fully understand this!
I just think, it will be more work in the future, if you introduce more names with the wrong naming convention (we have to deprecate it in the renaming phase, etc.)
Anyway: Your decision. This is not super important to me.
There was a problem hiding this comment.
Okay, I "unresolve" this for another point:
Some of the inner workings of FairGeoSet (especially the usage of the modules member variable) depend on maxModules not changing during operation. Providing a simple setter like this suggests that changing maxModules at any time is allowed.
Either this method should check that modules is not already allocated.
Or (preferred) this method should not exist at all and the maxmodules parameter should be part of the ctor.
If you don't want to go the ctor route in this PR, then maybe leave out the complete maxModules part at all and move it to another PR?
There was a problem hiding this comment.
BTW: The same is probably true for maxSectors?
| SECTION("name generation") | ||
| { | ||
| const Int_t index = GENERATE(0, 1, 99); | ||
| auto index = GENERATE(take(100, random(1, 1000))); |
There was a problem hiding this comment.
const auto does not work?
fuhlig1
force-pushed
the
improve_geo_classes
branch
from
41dc0cd to
bedee49
Compare
Remove usage of char* with std::string. Since the API requests to return a char* this information is extracted from the string. Add checks to confirm the correctness of the input. Add a accessor to set the maximum number of modules. The generation and parsing of the names uses the fmt library such that this dependency has to be added to the build system.
Instead of 3 hardcoded values use 100 random values in the range from 1 to 10000 to thest name generation and extraction in FairGeoSet and derived classes. Currently the tests of the example code fails since the code has an error with numbers which have mor than one digit. The correct code is already in the base class but currently shadowed by code in the example classes.
The functionality to generate and parse names is now available from the base class FairGeoSet. Use the base class implementation. Use functions in the constructor to initilaize data members instead of accessing the data members of the parent class directly. This will allow to make these data private in future.
The volume name violates the naming conventions. The problem only was found due to the newly added unit tests.
fuhlig1
force-pushed
the
improve_geo_classes
branch
from
bedee49 to
85f4ec3
Compare
|
We will address the requested changes for v19.2. |
4 participants
Improve the Geo classes in the example directory.
Move duplicate code into the base class FairGeoSet.
In the constructors use setter functions instead to access data members of the base classes directly.
Use fmt library for name generation and parsing to get rid of sprintf.
Add and improve unit tests for these classes,
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores