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

Port adapter to CCX v2.21 #115

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

numericalfreedom
Copy link

Update to CalculiX version 2.21 (July 2023)

@numericalfreedom
Copy link
Author

Compiles with preCICE version 2.5.0. PastiX solver not tested.

@uekerman uekerman changed the title Develop ccx 2 21 Port adapter to CCX v2.21 Nov 3, 2023
@uekerman
Copy link
Member

uekerman commented Nov 3, 2023

Thanks for the important contribution @numericalfreedom 🚀

Did you run any of the preCICE tutorials as tests or how did you check whether the adapter works correctly?
Let's try to document as much as possible.

@numericalfreedom
Copy link
Author

Dear Benjamin,

thank You for Your kind comments.

The preCICE version of ccx_2.21 compiles and works fine with all native CalculiX tests (standalone ccx_preCICE works fine).

Between the versions ccx_2.20 and ccx_2.21 there were only some cosmetic changes as far as it is concering the preCICE connection. I assume, that the version ccx_2.21 with preCICE works also fine but this remains to be thoroughly tested. I am too much a newbee to have several codes available to couple CalculiX with and also to have the detailed knowledge to do so. Maybe, I can use Your expertise on this.

Thank You very much for Your kind attention and best wishes,

Nandor
@numericalfreedom on Github

Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

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

Thank you for the update!

The Makefile needs some attention, but I managed to build the adapter using the same Makefile as in ccx2.20 (with only the version changed).

Besides that, I have the impression that the files ccx_2.21.c and CalculiX.h are the unmodified CalculiX files, not the adapted ones. I noticed that by trying to run a tutorial, in which case the CalculiX participant just started iterating without waiting for the other participant.

Let me know if you need any help applying my suggestions! :-)

Comment on lines -5 to +7
CCX_VERSION = 2.20
CCX = $(HOME)/CalculiX/ccx_$(CCX_VERSION)/src
CCX_VERSION = 2.21
CCX_HOME = /opt/src
CCX = $(CCX_HOME)/CalculiX/ccx_$(CCX_VERSION)/src
Copy link
Member

Choose a reason for hiding this comment

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

The CCX_VERSION should indeed be updated, but the CCX should remain as-is.
I don't think we need a separate CCX_HOME variable, it makes copy-pasting paths more complicated. If it helps further parts or CalculiX-internal workflows, let's keep it.

Comment on lines -10 to +31
SPOOLES_INCLUDE = -I/usr/include/spooles/
# SPOOLES library flags (e.g. $(HOME)/SPOOLES.2.2/spooles.a)
SPOOLES_LIBS = -lspooles
# SPOOLES_INCLUDE = -I$(CCX_HOME)/SPOOLES.2.2
SPOOLES_INCLUDE = -I/usr/include/spooles
# SPOOLES library flags (e.g. -L$(HOME)/SPOOLES.2.2/spooles.a)
# SPOOLES_LIBS = -L$(CCX_HOME)/SPOOLES.2.2 -l:spooles -L$(CCX_HOME)/SPOOLES.2.2/MT/src/spoolesMT -l:spoolesMT
# SPOOLES_LIBS = $(CCX_HOME)/SPOOLES.2.2/MT/src/spoolesMT.a $(CCX_HOME)/SPOOLES.2.2/spooles.a
SPOOLES_LIBS = -lspooles
#
# ARPACK include flags (e.g. -I$(HOME)/ARPACK)
ARPACK_INCLUDE =
ARPACK_INCLUDE = -I/usr/include/arpack
# ARPACK_INCLUDE = -I$(CCX_HOME)/ARPACK/SRC
# ARPACK library flags (e.g. $(HOME)/ARPACK/libarpack_INTEL.a)
ARPACK_LIBS = -larpack -llapack -lblas
# ARPACK_LIBS = -L$(CCX_HOME)/ARPACK -l:arpack_INTEL -llapack -lblas
# ARPACK_LIBS = $(CCX_HOME)/ARPACK/libarpack_INTEL.a -llapack -lblas
ARPACK_LIBS = -larpack -llapack -lblas
#
# yaml-cpp include flags (e.g. -I$(HOME)/yaml-cpp/include)
YAML_INCLUDE = -I/usr/include/
# YAML_INCLUDE = -I/usr/share/yaml-cpp/head/include
YAML_INCLUDE = -I/usr/include/
# yaml-cpp library flags (e.g. -L$(HOME)/yaml-cpp/build -lyaml-cpp)
YAML_LIBS = -lyaml-cpp
# YAML_LIBS = -L/usr/share/yaml-cpp/head/lib -lyaml-cpp
YAML_LIBS = -lyaml-cpp
Copy link
Member

Choose a reason for hiding this comment

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

Were these changes intended?
Let's keep the same format (number of spaces).

Comment on lines -45 to +54
$(PKGCONF_LIBS) \
$(PKGCONF_LIBS) \
Copy link
Member

Choose a reason for hiding this comment

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

In Makefiles, tabs have meaning, so we use them everywhere in the file instead of spaces.

Besides that, mixing tabs and spaces leads to some strange situations, e.g., in my editor this now looks like this:

Screenshot from 2023-11-08 13-32-23

@@ -53,6 +62,7 @@ LIBS = \
#FFLAGS = -g -Wall -O0 -fopenmp $(INCLUDES)

CFLAGS = -Wall -O3 -fopenmp $(INCLUDES) -DARCH="Linux" -DSPOOLES -DARPACK -DMATRIXSTORAGE -DUSE_MT
CFLAGS = -w -O3 -fopenmp $(INCLUDES) -DARCH="Linux" -DSPOOLES -DARPACK -DMATRIXSTORAGE -DUSE_MT
Copy link
Member

Choose a reason for hiding this comment

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

Why changing from -Wall to -w?
If it was intended to add the -Wextra, then let's just add it explicitly.

Or is this a change that comes from the CalculiX v2.21 Makefile?

Same below.

Comment on lines -89 to +102
g++ -std=c++11 $(YAML_INCLUDE) -c $< -o $@ $(LIBS)
#$(CC) $(CFLAGS) $(INCLUDES) -c $< -o $@ $(LIBS)
$(CC) -std=c++11 $(INCLUDES) -c $< -o $@ $(LIBS)
# g++ -std=c++11 $(YAML_INCLUDE) -c $< -o $@ $(LIBS)
# $(CC) $(CFLAGS) $(INCLUDES) -c $< -o $@ $(LIBS)
Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should change g++ to $(CC). I also like the additional space. Then maybe just remove the commented-out line 101.

Comment on lines 118 to +121
$(FC) -fopenmp -Wall -O3 -o $@ $(OCCXMAIN) $(OBJDIR)/ccx_$(CCX_VERSION).a $(LIBS)
# $(FC) -fopenmp -Wall -O3 $(OCCXMAIN) $(OBJDIR)/ccx_$(CCX_VERSION).a $(LIBS) -o $@

# $(LIBS)
Copy link
Member

Choose a reason for hiding this comment

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

Why this reordering and formatting?

Comment on lines -5 to +6
CCX = $(HOME)/CalculiX/ccx_2.20/src
CCX_HOME = /opt/src
CCX = $(CCX_HOME)/CalculiX/ccx_2.21/src
Copy link
Member

Choose a reason for hiding this comment

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

Similarly to the normal Makefile (reminder, independent of the decision to keep or frop the CCX_HOME.

@@ -0,0 +1,1923 @@
/* CalculiX - A 3-dimensional finite element program */
Copy link
Member

Choose a reason for hiding this comment

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

Comparing this file with ccx_2.20.c, it looks like this file does not contain the preCICE-specific changes. Did you maybe accidentally replace the complete file with the unmodified Calculix?

ITG *irow, ITG *jq, ITG *neq, double *adfreq, double *aubfreq,
ITG *irowfreq, ITG *iaux, ITG *jqfreq, ITG *icolfreq, ITG *neqfreq,
ITG *nzsfreq, double *om, ITG *symmetryflag, ITG *inputformat,
double *b, double *bfreq)
Copy link
Member

Choose a reason for hiding this comment

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

Similarly to the ccx_2.21.c, it looks like the preCICE-specific changes are missing.

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