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.

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

macOS support #139

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

macOS support #139

wants to merge 12 commits into from

Conversation

NRavoisin96
Copy link
Contributor

@NRavoisin96 NRavoisin96 commented Oct 24, 2024

SCONE Installation guide for macOS.docx

Dear all,

After longer than I would like to admit, I am happy to announce that SCONE now runs natively on macOS, both for Intel and ARM (Apple Silicon) machines. I have attached the documentation to correctly set up SCONE on macOS. A couple points to note:

  • Most of the issues reported by students seem to stem from improper configuration and / or compiler choices. Apple devices, by default, ship with Apple's in-house C compiler, known as Apple Clang. It is strongly recommended to use this compiler when installing pFUnit and SCONE, as other compilers such as the ones in gcc packages are not officially supported by Apple and will likely cause errors when trying to set up SCONE. As such, in the documentation it is required to use Apple Clang.
  • A special mention should be made regarding issue Installation Mac  #31: the reason for the unit tests failing in this case is due to rounding-off errors being slightly different on ARM-based Macs compared to machines (including Macs) running on Intel CPUs. This can be due to a combination of several factors, including different memory registry lengths and / or instruction set optimisations. In any case, the actual culprit causing plane test to fail is given here:
    k = dotProduct(u, self % norm)
    c = self % evaluate(r)
    if ( k == ZERO .or. abs(c) < self % surfTol()) then ! Parallel or at the surface
    ...
    Here, k (a floating point number) is tested to be equal to ZERO (another floating point number) to the bit. This is bad practice and dangerous. I have written two small helper functions in the genericProcedures.f90 module which test for equality between two floating point numbers using small tolerances instead. In the future, please make use of these instead of testing for perfect equality, unless you are 100% sure that the variable you are testing can be exactly equal to the specific value of interest. In general, any variable which results from mathematical operations should never be tested for perfect equality. I have corrected instance of such tests in the Geometry section, although I have seen it happen in many other sections. As I am less familiar with the code in these sections, I would advise people working on them to have a look and assess when exact equality can be tolerated and when it needs to be replaced with the helper functions.
  • Fortran comes with an intrinsic dot_product function so there is no need to define our own dotProduct function in the genericProcedures.f90 module. I have removed it in this pull request.

Many thanks,
Nathan

@NRavoisin96 NRavoisin96 marked this pull request as ready for review October 24, 2024 11:22
@NRavoisin96 NRavoisin96 mentioned this pull request Oct 24, 2024
@ChasingNeutrons
Copy link
Collaborator

Thanks for doing this @NRavoisin96, it has been long overdue. I came across that dotProduct recently and wondered why we didn't use dot_product - probably because it is some quite old code and we didn't know any better! Likewise, thanks for the 'areEqual', that has also bugged me in the past.

The code updates look great. The only thing is to update the docs. That is, the contents of your guide should be incorporated into docs/Installation.rst, rather than as a separate document which may get lost on closing this PR.

@NRavoisin96
Copy link
Contributor Author

@ChasingNeutrons,

The documentation has been updated. I have also used this opportunity to fix some small typos in the installation.rst file and added a section at the on how to actually run SCONE (using the JEZ input file as an example), so that the required syntax to execute SCONE is actually included somewhere in the documentation.

Many thanks,
Nathan

Copy link
Collaborator

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski left a comment

Choose a reason for hiding this comment

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

Sorry it took me so long to get around to this.
In addition to the minor comments/questions I have one major issue. I don't think it is a good idea to make the surfaces even fuzzier due to the one unit test failing. Especially that it fails mostly because it is too strict. The distance calculated on M# chips is still arguably correct (is supposed to be very large, and it is). I think we can just allow for more leeway in the test to solve our problem. My fear is that reasoning about edge cases in surfaces is quite tricky as it is. Making the comparisons not exact can make that problem worse.

That being said the generic function for FP equality is welcome and should be integrated.

!! tolerances to assert equality between two floating point numbers. These tolerances
!! are specified in numPrecision.f90.
!!
elemental function areEqual_defReal(value, target) result(equal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be inclined to change the name. I have a feeling that isEqual is a bit confusing since the numbers are not really equal... I feel isApproxEqual may be more appropriate.

! Compute the absolute value of the difference between the two floating point numbers.
! Note that if value and target are both very large and of opposite signs this can
! cause overflow.
absDiff = abs(value - target)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The floating point approximate equality is always a iffy subject...
If we try to write a generic procedure for the future I would be inclined to:

  • accept the tolerance value as arguments. If we wish we can make them optional and set default values. It is always a bit annoying to implement in Fortran but... what else can we do?
  • I would check both the absolute and relative tolerance. When it comes to how to combine them in the best way it is always a bit unclear to me. I think or is the most appropriate though (which is what you did).

Comment on lines +625 to +630
if (absDiff < floatTol) return

! Check if value and target are within some small relative tolerance of each other and
! return if yes. Note that if value and target are both very small numbers, then multiplying
! by a small tolerance can cause underflow. This is why we check absolute tolerance first.
if (absDiff < max(abs(value), abs(target)) * FP_REL_TOL) return
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be annoying and just assign the return value to some logical expression. Along the lines of:

absolute = absDiff < floatTol
relative = absDiff < max(abs(value), abs(target)) * FP_REL_TOL 
equal absolute .or. relative 
return

But it may be just my personal taste so feel free to ignore me.

!! tolerances to assert equality between two floating point numbers. These tolerances
!! are specified in numPrecision.f90.
!!
pure function areEqual_defRealArray(array, target) result(equal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this one may not be necessary? In particular I fear it may cause confusion with the elemental version. For checking arrays I would prefer to call the elemental version and feed the output to all intrinsic procedure.


mkdir build
cd build
cmake ./..
make tests
make install

#. Export environmental variables required by pFUnit::
#. Export environmental variables required by pFUnit in your ``.bashrc`` file::
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#. Export environmental variables required by pFUnit in your ``.bashrc`` file::
#. Export environmental variables required by pFUnit::

I mean... they are just required by the installation. No need to mess with .bashrc I think.

brew install gcc cmake python git openblas lapack libomp

#. Close your `Terminal` window. Open a new `Finder` window and navigate to your `Home` directory
(⌘ + ⇧ + h). Display hidden files (⌘ + ⇧ + .) and find the ``.zprofile`` file (this is the macOS
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️ unicode!

following lines which are not already present** (note: this depends on whether you have a Mac
running on an Intel CPU or an ARM - Apple Silicon - chip):

* Intel::
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't quite follow why there are two sections based on the CPU ISA?
The only difference seem to be the install path of the homebrew package manager. Maybe we can combine both sections (and just point out the difference at the start) to minimise repetition?

Comment on lines +257 to +261
#. Before proceeding, **make sure that the default C compiler is Apple Clang by entering
the following command**::

gcc --version

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow this part. If we verify clang why do we want to invoke gcc? Is this a typo?

* Initialise CMake by specifying which C compiler to use. In your `Terminal` window enter
the following::

cmake -D CMAKE_C_COMPILER=CLANG ./..
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure it needs to be capitalised? I though it was supposed to be a path or command that invokes the compiler. Hence I was expecting clang...

Also I am not sure why are the steps with gcc above necessary. Sorry, I got a bit lost here.

Comment on lines +431 to +436
file or, better yet, export this variable in your ``.bashrc``/``.zprofile`` file (depending on your OS) by adding the
following line::

export SCONE_ACE=path_to_fullLib.xsfile

and saving the changes in either case. Note that if you export this variable in your ``.bashrc``/``.zprofile`` file, you
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't insist on setting the environmental variables in the .basrc. I think it is fine to export them before running. It makes (should make) one more conscious of the nuclear data they use ;-)

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