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.

Sign up for GitHub

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

Jump to bottom

Does this project accept contributions? #2

Open
per-gron opened this issue Dec 28, 2019 · 17 comments
Open

Does this project accept contributions? #2

per-gron opened this issue Dec 28, 2019 · 17 comments

Comments

@per-gron
Copy link

per-gron commented Dec 28, 2019 •
edited
Loading

Hi,

I'm trying to use Radia in a project I'm working on. While working on integrating Radia with my project (which is in C++ fyi) I have run into some issues with Radia's code, at this point mostly near-mechanical C++ code quality issues, some of which have required me to fork the Radia code, which I would like to avoid if at all possible.

Does Radia accept contributions? I would like to upstream my fixes.

Here is a list of issues that I have found:

Surface-level code issues:

  • X using namespace std; in header files. This spreads like a virus across any larger code base and can be extremely expensive to undo (I've seen this happen at Spotify and Google, where it cost a lot). Best practice is to not use using (except for type aliases) in header files.
  • X Reading of uninitialized variables (both ones caught by compiler warnings and ones that aren't). This often causes memory corruption bugs. For example, there are cases in ~radTPolyhedron where pJ_LinCoef is uninitialized.
  • X Some memory leaks. (Reported by address sanitizer.)
  • X Classes with virtual methods but not a virtual destructor. Best practice is to always mark the destructor of any class that has virtual methods virtual. Failing to do so can cause memory leaks and other really nasty issues because it's very easy to accidentally call the wrong destructor.
  • Strange use of #include in the middle of code in radg3d.h. If you need speed from inlining it's usually better to use link-time optimization than hacks like this. inline doesn't actually tell the compiler to inline the function anyways. This hack causes radtrans.cpp and other files that include radtrans.h to not compile. I worked around this by not building radtrans.cpp but it seems very strange to me, I would prefer if radTg3d::GetTrfAndCenPointInLabFrame simply was in radg3d.cpp.
  • Unused private fields, unused local variables.
  • Comparisons that always evaluate to true.
  • Implicit conversions that Clang warns about.
  • Other miscellaneous warnings that make clang fail when compiling with -Wall -Werror.
  • Code that is commented out with no explanation, left to rot and confuse people like me.
  • Unsafe use of strcpy.

Issues that are a bit more work to fix:

  • Lack of const correctness. (Both as in literal lack of const keywords and in a deeper sense, where parameters marked as input are modified by the function.)
  • Indices that begin with 1 in C++ code (it seems?), without documenting this fact.
  • Explicit memory management (new/delete); since C++11's introduction of unique_ptr there is virtually never a need for that, and it's bug prone as the memory leaks suggest.
  • Extensive use of raw pointers in code that does not clearly document important invariants (nullability, is it an array, what is the length of the array, who is the owner). This is typically fixed by using higher level types, for example: Never pass ownership via raw pointer, use things like unique_ptr instead. Use a type that indicates that it is an unowned array including size instead of a raw pointer and separate length parameter. Use references whenever the parameter is assumed to not be null.
  • Lack of tests (or am I missing them?)

Points marked with X are the ones that are most important to me personally as they are the ones that I feel require me to fork the code. I cannot use Radia without fixing them. It would be great if you would accept fixes at least these, or even better for the "surface level code issues" list. I'm not sure I will have time to actually work on the second list.
@ochubar
Copy link
Owner

ochubar commented Dec 29, 2019

Hi,
Thanks for trying to use Radia, the 3D Magnetostatics code that is in use by Community since 1997.
I appreciate your comments, and will try to look into some of them, other priorities permitting.
Here are a few general "comments on your comments".

  • We try to write C++ code that compiles with Visual C++, GCC and Xcode, which makes our code cross-platform. These compilers typically support different levels/standards of C++, with more recent language features being usually less supported; therefore we prefer to use the most general (rather than most recent) features. This strategy has proven to work well over decades, resulting in the code that doesn't require a lot of maintenance. The things like "using namespace std;" were introduced in the code before the STL became part of the C++ standard library; and we also e.g. had to introduce our "smart pointers" (based on Stroustrup's example) for the same reason. Our code may look not very modern, not very "clean", and not very easy to be integrated into a more recent C++ project; however, as far as the code compiles with the above-mentioned compilers, we don't see a big need to modify it. Rather, we would prefer to apply our efforts to extending code's functionality, performance, and availability: e.g. some efforts the currently dedicated to MPI- and OpenMP-based parallelization, as well as to improving interfaces, such as PyRadia.
    If you strongly dislike some places of our C++ code (yet still would like to use it ;) ), you can try to make some "pull request", preferably after trying compilation with Visual C++, GCC and Xcode (on Windows, Linux and Mac OSX respectively); we'll consider your suggested changes.

  • Memory leaks are very important issues, and we would apply all possible efforts to fix these. However, these issues have to be illustrated and tested practically, by some code example(s), and not only by some "address sanitizer" (because what it may report as a bug may be actually a normal processing in the code, or something which never results in true memory leak).

  • Lack of tests (or am I missing them?)
    Google-ing "A 3D Magnetostatics Computer Code for Insertion Devices" returns for me ~346 citations; "Computing 3D Magnetic Fields from Insertion Devices" ~226 citations. De-facto, there can be more than that, because we never explicitly requested referencing Radia in publications. In all these cases, the Radia C++ code was probably used through its Mathematica interface. We are working now on the Python interface. The Radia C API is probably used not very often; and the C++ files without the C API were probably ~never used in a serious project so far.
    BTW, why can't you use the Radia C API and a compiled library in your project, instead of modifying the C++ files?

  • "Cloning" the code certainly doesn't sound to me like a good idea; simply because you will likely to "miss" a number of new features we are working on now, thanks to an active grant (or you'll have to do some regular "ports" of the code).

@per-gron
Copy link
Author

Thanks for the response!

I completely understand that Radia is an old project with a legacy, and the requirement to be cross platform. I appreciate this aspect of the library, as I am able to run it on Linux now, and might later compile it to run in browsers with webassembly. As I mentioned, I use Clang (which is used by Xcode) now, but I have worked on code bases which target all of the compilers you mention at once.

Do you know which versions of those C++ compilers that you are targeting? The features that I most would like to use, in particular unique_ptr were introduced in C++11, which is two entire C++ standards old. Even Visual C++ which traditionally has lagged behind quite a bit has had full C++11 support for a long time now.

Regarding the memory leaks: I'll send some PRs with fixes, I have fixed 3 so far. Address sanitizer is a very accurate tool which is designed to never report an error unless there definitely is a bug.

When I mentioned tests I meant automated tests. It is generally accepted in the software industry that it is very important to write programs that run the code and test various aspects of the code. Such test can be unit tests, integration tests etc. They take some time to write in the first place, but for pieces of software that are meant to be maintained over time, they save a lot of effort in the long run.

BTW, why can't you use the Radia C API and a compiled library in your project, instead of modifying the C++ files?

Building the Radia code from source makes certain things much easier, including debugging, running sanitizers for finding bugs, porting to different platforms etc. The project I'm working on uses many libraries and building everything from source helps keep the complexity down.

The changes I have done to Radia code are rather unobtrusive.

@ochubar
Copy link
Owner

ochubar commented Dec 29, 2019

Hi,
I develop on Windows, always using the ~most recent version of Visual C++ (currently in VS 2019, community edition), and then compile on GCC, and after this on Xcode (often with someone's help). I am a physicist (very busy with multiple scientific projects, not a software engineer or computer scientist), and I'm sure I woduld never manage to develop Radia and my other codes - for the amount of time I spent on this - if I wouldn't use Visual C++ as my main dev. tool.

Regarding the memory leaks: I'll send some PRs with fixes, I have fixed 3 so far.

I'm looking forward to seeing these. I didn't face issues with memory leaks in Radia over the last ~20 years :), ...but that's probably because I'm mainly using only a small subset of functions; and there may be issues with functions that were added ~recently (i.e. after 2006 :)).

I wonder, what's your background, and why do you need Radia (I know a few colleagues at PSI in Switzerland who like and make use of Radia since a while, but you seem to be from a different area?).

When I mentioned tests I meant automated tests. It is generally accepted in the software industry that it is very important to write programs that run the code and test various aspects of the code...

In physics, validity / accuracy test of a simulation code usually consists in comparing its simulation results with measurements made on real systems / devices; the other aspects are often much less important. :) …But I appreciate your comments and maybe assistance in the software engineering aspects, if these will result in improvements of the code and maybe productivity.

O.Chubar
tenure physicist
National Synchrotron Light Source II
Brookhaven National Lab, USA

@per-gron
Copy link
Author

I develop on Windows, always using the ~most recent version of Visual C++ (currently in VS 2019, community edition), and then compile on GCC, and after this on Xcode (often with someone's help)

Ok! Visual C++ has supported almost all (including unique_ptr) of C++11 since version 2015, and GCC/Clang have supported C++11 pretty much since the standard was published. If you don't need to support old compilers (for example because you're targeting old versions of Windows or embedded devices with proprietary compiler toolchains) I think you should be able to use C++11 with no issues. C++11 introduced several features that make life much easier.

I didn't face issues with memory leaks in Radia over the last ~20 years :) [...]

The memory leaks I have found are not big ones. I care about them mostly because I want to use Address Sanitizer for everything, and it's much easier to see regressions if there are zero errors.

I wonder, what's your background,

I'm a software engineer. I currently work at Google, and before that I worked at Spotify. I don't do any science-y or math-y work there, but have done a lot of C++.

and why do you need Radia (I know a few colleagues at PSI in Switzerland who like and make use of Radia since a while, but you seem to be from a different area?).

I am working on (not for Google) a project that involves simulating the behavior of electric guitar pickups, which are magnetic sensors. Radia is designed to do much more advanced things than this, but it is the most suitable piece of software that I have been able to find. I need to simulate magnetic fields in 3D of permanent magnets.

In physics, validity / accuracy test of a simulation code usually consists in comparing its simulation results with measurements made on real systems / devices; the other aspects are often much less important. :) …But I appreciate your comments and maybe assistance in the software engineering aspects, if these will result in improvements of the code and maybe productivity.

I see. Since it's not obvious if there is an error by just looking at the output things get a bit more tricky than typical software. I think some tests that just run as much of the code lines as possible that don't actually look at the answer, but that can be run with sanitizers, could help catch plenty of basic mistakes such as array out of bounds accesses etc. If you can get people who use this software to contribute code that runs their experiments, it could make sense to have these as tests that verify that the output after a code change doesn't change too much compared to the output that was there before (this only works for simulations that run in reasonable amounts of time, perhaps <30 minutes each maybe, ideally <10 mins).

@per-gron
Copy link
Author

How is Radia compiled with Xcode/Mac? Do you use make? I can't find an Xcode project (except ext_lib/igor/XOPSupport/Xcode/XOPSupport.xcodeproj which doesn't seem to contain the whole project).

@per-gron
Copy link
Author

Would you be interested in a Pull Request that integrates Radia with Travis CI?

Travis is an online tool. With very little configuration it can be configured so that for every pull request and every commit to the master branch it will build the code (and run tests if they exist) and report the results back in the PR. It supports Linux, Windows and Mac, so it can be set up to compile every change on every platform and compiler toolchain that Radia supports. And it's free for open source projects.

If you are currently checking that the code works on different operating systems manually, it seems like this could save you quite a bit of time.

@ochubar
Copy link
Owner

ochubar commented Dec 30, 2019

Hi!
Thanks for the pull requests.

Visual C++ has supported almost all (including unique_ptr) of C++11 since version 2015,...

It's a too short term on my scale.:) I have a habit (and pleasure) programming via pointers, and often need advanced memory management, so I'll wait until pointers will be announced "deprecated" in C++ (if this will happen before I stop programming :)).
Thank you for pointing me to these C++ updates. It is certainly good to see how it evolves.

I am working on (not for Google) a project that involves simulating the behavior of electric guitar pickups, which are magnetic sensors. Radia is designed to do much more advanced things than this, but it is the most suitable piece of software that I have been able to find. I need to simulate magnetic fields in 3D of permanent magnets.

Interesting. Note however that Radia is a magnetostatics code; calculating static 3D magnetic field is easy with it, but as is, it won't allow you to directly simulate time-/frequency-dependent phenomena (e.g. electromagnetic induction or Eddy currents); some "post-processing" may need to be done in your case. We with colleagues keep in mind extending Radia in these directions, benefiting from fast solving of static / steady-state problems (especially after we implement parallelization), but this is a relatively big effort (that may require a good motivation).

If you can get people who use this software to contribute code that runs their experiments, it could make sense to have these as tests that verify that the output after a code change doesn't change too much compared to the output that was there before (this only works for simulations that run in reasonable amounts of time, perhaps <30 minutes each maybe, ideally <10 mins).

We use standard Radia examples for this purpose (there are ~7 in the Mathematica and Python versions). I know what their results should be, but nothing is automated. Perhaps something can be done on this basis(?); any references will be strongly appreciated!

How is Radia compiled with Xcode/Mac? Do you use make?

We did not yet update the Xcode project to compile a recent Radia shared / static library and Py version for Mac; there is only an out-of-dated Xcode project for compiling Radia for Mathematica, but we plan to update it. The easiest at this point would probably be to take the Makefile for Linux and modify it for Mac.

@dhidas
Copy link

dhidas commented Dec 30, 2019 via email

@ochubar
Copy link
Owner

ochubar commented Dec 30, 2019

Would you be interested in a Pull Request that integrates Radia with Travis CI?

Thanks already for the reference! Let me first check with our guys in the lab; I thinks they mentioned this thing too (I'd like to be "compatible").

@per-gron
Copy link
Author

I have a habit (and pleasure) programming via pointers,

Pointers are not going away ever in C++. With modern C++ features like unique_ptr the pointers are still there, it's just that the compiler does a lot of the tedious but difficult work for you. For example, the data() method on std::vector gives you a raw pointer to the underlying array, but std::vector gives you nice things like automatic memory management, the ability to resize the array, and it automatically calls destructors and similar on its elements. It's got all the nice things of the C++ of old, but it does things like writing the code in radTInteraction::EmptyVectOfPtrToListsOfTrans() automatically for you, and it doesn't make off-by-one errors.

It takes a little bit of time to learn how to use the new features, but some of them, especially the ones around automatic memory management, are well worth it. (Others, like template metaprogramming features are not necessary to know most of the time.) When you don't have to worry about mechanical things like when to write delete you are free to focus on other things that are specific to your application.

and often need advanced memory management,

I think you'd be surprised by how much ground is covered by unique_ptr, shared_ptr, vector and unordered_map/set. And the nice thing is: If you do need something more advanced, like arena allocators, graphs with cycles etc, you can still write them exactly like before, just don't use these classes then.

so I'll wait until pointers will be announced "deprecated" in C++

You might be interested to see that the C++ Core Guidelines which is a set of code style guidelines written by Bjarne Stroustrup and published by the Standard C++ Foundation recommend to not call new and delete explicitly.

@ochubar
Copy link
Owner

ochubar commented Dec 30, 2019

Hi Dean, thanks for the update.
I wonder, does triangulation still work after the changes you had to do? Perhaps you can check this with Example #7 on Mathematica (or I can send you its Py version soon)? If all examples seem to work fine - don't hesitate to make a pull request.

@per-gron
Copy link
Author

If you can make the Makefiles work on macOS, it should be easy to make it run on Travis. I don't have easy access to a Mac right now so it is a bit difficult for me to tweak the makefiles to work there.

We use standard Radia examples for this purpose (there are ~7 in the Mathematica and Python versions). I know what their results should be, but nothing is automated. Perhaps something can be done on this basis(?); any references will be strongly appreciated!

Using the examples as a small automated test suite sounds like a good idea to me! If you want the tests to be written in Python, you should be able to use the unit testing framework that is built into Python. It's also possible to write the tests in C or C++.

Automated tests (at least the kind that runs on a single computer) are at the core usually nothing more than a command line program that exits with code 0 on success and non-0 on failure. Then you have some kind of test runner that runs these programs and tells you if any of the tests failed.

Note however that Radia is a magnetostatics code; [...]

In the project I'm working on, running the simulations in real-time is an absolute requirement, accuracy is not. Because of this, even if Radia gets support for solving non-static problems, I don't think it would be directly useful for me. I want to use Radia to compute the magnetic field of a guitar pickup "at rest" from a 3D model of permanent magnets and iron/steel bars/slugs. For the actual time-domain processing we will need to resort to very simplified models that use the static "at rest" data as input.

@dhidas
Copy link

dhidas commented Dec 31, 2019 via email

@robnagler
Copy link

@dhidas we have developed a VTK.js integration for Radia and converted RADIA_Example05

It's on our beta server now, but as soon as it is approved (probably next week), we'll move it to jupyter.radiasoft.org.

It is available as a Docker image (alpha or beta tags) with a curl installer:

curl https://jupyter.run | bash -s beta

Docker needs to be installed first.

@youzheho
Copy link

Dear all:

I have one issue that i cannot run the radia om mac os catalina(version.10.15.5)

Did anyone who can help me step by step?
my email is [email protected]
Thanks
You Zhed

@lufermar
Copy link

Has anyone solved the problem that youzheho had? I also cannot execute Radia.exe on Mac and I would appreciate some help:)

@robnagler
Copy link

@lufermar you can use Radia via Docker. RadiaSoft offers several public images with Radia (and many other codes) builtin:

  • beamsim is for command line users
  • beamsim-jupyter can be used to a local jupyter instance which also has our 3d renderers (look earlier in the thread)

You can also access Radia online for free via Sirepo Radia, which is a graphical front-end for Radia. RadiaSoft's free JupyterHub server allows you to run Radia via the command line or a notebook.

Compiling Radia on the Mac will likely require some expertise with Homebrew. You can probably install the compilers and then look over RadiaSoft's build script for Linux. At RadiaSoft, we use Macs, and run Radia via VirtualBox, JupyterHub, and Docker. We do not compile on our Macs directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@per-gron @ochubar @robnagler @dhidas @youzheho @lufermar